Skip to content

Kawai K5000 #433

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

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Kawai K5000 #433

wants to merge 22 commits into from

Conversation

christofmuc
Copy link
Owner

Bank management and sending from kk to synth currently NOT working

@christofmuc
Copy link
Owner Author

I need to drop for today, but my main suggestion is that we add a proper make_test_data function here and enable you to run and debug it - this is how I approach harder problems where there might be some bugs lurking. Running this from the Orm itself is much harder to figure out what is going on. I first get the module as a standalone to pass the test before I first try it in the Orm!

@christofmuc
Copy link
Owner Author

I have added and pushed the test function, you might want to revisit the adaptation testing guide for this (long time I have looked at it).

When I run the test (it is only one for now), I get this error:


message = [240, 64, None, 32, 0, 10, ...]

    def isSingleProgramDump(message):  # ✅
        # Check minimum length to avoid out-of-bounds errors
        if len(message) < 10:
            return False
    
        # Verify Sysex header for a Kawai K5000 program dump
        return (message[0] == 0xF0  # Start of SysEx
                and message[1] == KawaiSysexID  # Kawai manufacturer ID
>               and 0x00 <= message[2] <= 0x0F  # MIDI channel (0-F for ch 1-16)
                and message[3] == OneBlockDump  # Function ID for single dump
                and message[4] == 0x00  # Reserved
                and message[5] == 0x0A
                and message[-1] == 0xF7)  # End of SysEx
E       TypeError: '<=' not supported between instances of 'int' and 'NoneType'

KawaiK5000.py:146: TypeError

This should be debuggable.

You would need to create a run configuration in Pycharm for pytest, and use these parameters:

grafik

@christofmuc
Copy link
Owner Author

Don't forget to pull my changes, and I'm off to bed now!

@markusschloesser
Copy link
Collaborator

Don't forget to pull my changes, and I'm off to bed now!

will do! me too :-)

@christofmuc
Copy link
Owner Author

There is a None channel in the patch produced by your bank loaded:

grafik

@markusschloesser
Copy link
Collaborator

There is a None channel in the patch produced by your bank loaded:

grafik

but i don't understand why? can't def extractPatchesFromAllBankMessages(messages, channel=None) be used with channel? All other adaptations using extractPatchesFromAllBankMessages don't use channel (but it's not that many)

@markusschloesser
Copy link
Collaborator

also commited and pushed one single sound sysex to testdata

@markusschloesser
Copy link
Collaborator

also you probably guessed it already, but ✅ signals function working, ❌ function not working

@christofmuc
Copy link
Owner Author

christofmuc commented Feb 25, 2025

but i don't understand why? can't def extractPatchesFromAllBankMessages(messages, channel=None) be used with channel? All other adaptations using extractPatchesFromAllBankMessages don't use channel (but it's not that many)

Channel/Sysex ID is confusing admittedly, but the main point here is to not put in None, but a 0. I made a comment on the code in line 367 where you use the channel's default value None. I think this should be 0 (which could be interpreted as sysex ID 1 or Midi channel 1 or whatever=

@markusschloesser
Copy link
Collaborator

markusschloesser commented Feb 25, 2025

but i don't understand why? can't def extractPatchesFromAllBankMessages(messages, channel=None) be used with channel? All other adaptations using extractPatchesFromAllBankMessages don't use channel (but it's not that many)

Channel/Sysex ID is confusing admittedly, but the main point here is to not put in None, but a 0. I made a comment on the code in line 367 where you use the channel's default value None. I think this should be 0 (which could be interpreted as sysex ID 1 or Midi channel 1 or whatever=

ok, this fixes that one specific error, now we "just" need to fix the offset calculation 😂
but also i think the first patch is wrongly extracted (wrong name)

00:42:36: info Adaptation: base_ptr calculated: 260
Pointer table has 126 pointers
Processing patch index 9: tone_ptr=0
clean up..................
00:42:36: info Got 1 new or changed patches, saved to database

…tter (old code was plain wrong) but now throws errors when hitting namefromdump
@markusschloesser
Copy link
Collaborator

New code better, but still fails.
Does the flow go from extract bank to issingleprogramdump?

@christofmuc
Copy link
Owner Author

New code better, but still fails. Does the flow go from extract bank to issingleprogramdump?

You can see what is going on and what the expectations are by looking at test_adaptations.py, lines 401+

Each patch returned by extractPatchesFromAllBankMessages() has to be accepted by either isSingleProgramDump() or isEditBufferDump(), defined by a flag in the TestData() message returned by make_test_data(). single program dump is default.

So the expectation is that the bank loader produces something that is accepted by isSingleProgramDump().

bank_byte = bank_byte_map[bank_number]

# Reconstruct SysEx message with updated bank and program number
return message[:7] + [bank_byte, patch_number] + message[8:]
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks bogus - you split the message into 7 bytes at the front, and everything starting with byte 8 in the end. I'd think the message is one byte longer after this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks bogus - you split the message into 7 bytes at the front, and everything starting with byte 8 in the end. I'd think the message is one byte longer after this?

true, will look at it later in detail.
But do we actually need the function for this adaptation? synth has no edit buffer and extractbank stuff now does its own reconstruction

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also you did see my commits from today?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saw them, but didn't check them yet. Probably Saturday!

The convertToProgramDump() is required for the bank functionality, so yes, this needs to be implemented. Should be possible?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saw them, but didn't check them yet. Probably Saturday!

👍

The convertToProgramDump() is required for the bank functionality, so yes, this needs to be implemented. Should be possible?

why? or where is the sysex header reconstruction supposed to be happening? right now I'm doing formatted_patch = [ 0xF0, KawaiSysexID, 0x00, OneBlockDump, 0x00, 0x0A, 0x00, bank_byte, patch_number, checksum ] + list(current_patch) + [0xF7] at the end of extractPatchesFromAllBankMessages

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commit from today fixes this (confirmed successful write by synth)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

convertToProgramDump() is called when pressing the patch button - as we cannot send to edit buffer when edit buffer is not implemented, the system falls back to patch the last program of the first bank into the program dump and send that to the synth.

So the function ought to inject the correct program position and potentially correct sysex ID/MIDI channel into the patch data before it is sent out.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most adaptations now choose to store either edit buffers or program buffers in the database, and convert them on sending into the appropriate format. So extactPachesFromBank is good to create full program buffers, the convert... function really only alters the program place then.

@markusschloesser
Copy link
Collaborator

new code committed, though still not working 🙄

@markusschloesser
Copy link
Collaborator

@christofmuc don't check anything for extractPatchesFromBank, got lots of new (uncommitted) code for that. Should be done later this weekend

…rent banks. Still lots of debug print in there. And kk crashes when clicking on a patch
@christofmuc
Copy link
Owner Author

@markusschloesser
Copy link
Collaborator

Check push from last night 😁

@christofmuc
Copy link
Owner Author

Perseverance!

KnobKraft would crash if you return an empty bank descriptor array - it tries to calculate the program position for sending the patch into, the last number of the first bank, but if there are no banks, it'll crash right now. Can fix that, but that's for me because it did not detect a K5000 and didn't know the submodel.

@markusschloesser
Copy link
Collaborator

Code looks pretty good, except I didn't dig into the big big function yet. The rest is clear. I added a program test data generator so you get better test coverage, and they all pass, which is good. calculateFingerprint is still missing for proper deduplication.

did you add to the k5000 branch?? Can only see it in main, which has the old code

@christofmuc
Copy link
Owner Author

Code looks pretty good, except I didn't dig into the big big function yet. The rest is clear. I added a program test data generator so you get better test coverage, and they all pass, which is good. calculateFingerprint is still missing for proper deduplication.

did you add to the k5000 branch?? Can only see it in main, which has the old code

I did, this is the commit from me: 727a623

Did you update?

@markusschloesser
Copy link
Collaborator

Code looks pretty good, except I didn't dig into the big big function yet. The rest is clear. I added a program test data generator so you get better test coverage, and they all pass, which is good. calculateFingerprint is still missing for proper deduplication.

did you add to the k5000 branch?? Can only see it in main, which has the old code

I did, this is the commit from me: 727a623

Did you update?

must have been a glitch in pycharm, now new and merged code pushed

@markusschloesser
Copy link
Collaborator

markusschloesser commented Mar 4, 2025

2 Problems:

  1. with the current code, when I select to import Bank D from Menu/Midi/Import patches from synth. It sends the correct Bank ID (0x02), but while importing it shows "importing Bank E". Same for when requesting Bank E. 0x03 gets send, import dialogue shows Bank F. Bank A works fine. Requesting Bank F doesn't do anything.
    Why is that?
    I "think" it just to work before I started using bankDescriptors in createBankDumpRequest

  2. Now when I select Bank D in the "In synth" tree, I get

00:53:27: error Adaptation[KawaiK5000]: Error calling createBankDumpRequest: ValueError: Invalid bank identifier '1'. Available banks: ['Bank A', 'Bank D', 'Bank E', 'Bank F']

At:
  C:\Users\Markus\KnobKraft-Adaptations\KawaiK5000.py(270): createBankDumpRequest

00:53:29: error Giving up retrying initiating bank dump

When I click on Bank E, it actually gets Bank D but fails with the foreign key error.
Why does that not honor the IDs from bankDescriptors or why is it different from getting Bank via Menu/Midi/Import patches from synth

@markusschloesser
Copy link
Collaborator

oh and could you please check if I implemented the fingerprinting correctly?
Also the build pipeline and automated testing fails for test_rename_should_not_change_fingerprint. Does that mean that my fingerprinting is wrong, or the rename function?

Expected :'97fdbd2fba69e9dd8eed504f1966b785'
Actual   :'1bc615041a13e9c27b97d74c1d498d19'
<Click to see difference>

adaptations\test_adaptations.py:146 (test_rename_should_not_change_fingerprint[KawaiK5000.py])
'1bc615041a13e9c27b97d74c1d498d19' != '97fdbd2fba69e9dd8eed504f1966b785'
adaptation = <module 'KawaiK5000.py' from 'KawaiK5000.py'>
test_data = TestData(sysex='testData/Kawai_K5000/full bank A midiOX K5000r.syx', program_generator=<function make_test_data.<local...n_convert_program_to_edit_buffer=True, can_convert_edit_buffer_to_program=True, rename_name=None, not_idempotent=False)
    @require_implemented("nameFromDump")
    @require_implemented("renamePatch")
    @require_implemented("calculateFingerprint")
    @require_testdata("programs")
    def test_rename_should_not_change_fingerprint(adaptation, test_data: testing.TestData):
        for program in test_data.programs:
            if program.dont_rename:
                continue
            binary = program.message.byte_list
            new_name = get_rename_target_name(program, test_data)
            renamed = adaptation.renamePatch(binary, new_name)
            # This should not change the fingerprint
>           assert adaptation.calculateFingerprint(renamed) == adaptation.calculateFingerprint(binary)
E           AssertionError: assert '1bc615041a13e9c27b97d74c1d498d19' == '97fdbd2fba69e9dd8eed504f1966b785'
E             - 97fdbd2fba69e9dd8eed504f1966b785
E             + 1bc615041a13e9c27b97d74c1d498d19
test_adaptations.py:159: AssertionError

@christofmuc
Copy link
Owner Author

oh and could you please check if I implemented the fingerprinting correctly? Also the build pipeline and automated testing fails for test_rename_should_not_change_fingerprint. Does that mean that my fingerprinting is wrong, or the rename function?

Expected :'97fdbd2fba69e9dd8eed504f1966b785'
Actual   :'1bc615041a13e9c27b97d74c1d498d19'
<Click to see difference>

adaptations\test_adaptations.py:146 (test_rename_should_not_change_fingerprint[KawaiK5000.py])
'1bc615041a13e9c27b97d74c1d498d19' != '97fdbd2fba69e9dd8eed504f1966b785'
adaptation = <module 'KawaiK5000.py' from 'KawaiK5000.py'>
test_data = TestData(sysex='testData/Kawai_K5000/full bank A midiOX K5000r.syx', program_generator=<function make_test_data.<local...n_convert_program_to_edit_buffer=True, can_convert_edit_buffer_to_program=True, rename_name=None, not_idempotent=False)
    @require_implemented("nameFromDump")
    @require_implemented("renamePatch")
    @require_implemented("calculateFingerprint")
    @require_testdata("programs")
    def test_rename_should_not_change_fingerprint(adaptation, test_data: testing.TestData):
        for program in test_data.programs:
            if program.dont_rename:
                continue
            binary = program.message.byte_list
            new_name = get_rename_target_name(program, test_data)
            renamed = adaptation.renamePatch(binary, new_name)
            # This should not change the fingerprint
>           assert adaptation.calculateFingerprint(renamed) == adaptation.calculateFingerprint(binary)
E           AssertionError: assert '1bc615041a13e9c27b97d74c1d498d19' == '97fdbd2fba69e9dd8eed504f1966b785'
E             - 97fdbd2fba69e9dd8eed504f1966b785
E             + 1bc615041a13e9c27b97d74c1d498d19
test_adaptations.py:159: AssertionError

The assumption is that the fingerprint stays the same after renaming, so fingerprinting should blank out the name before hashing, or not included the name bytes at all. Rename is tested separately and seems to work.

@markusschloesser
Copy link
Collaborator

2 Problems:

  1. with the current code, when I select to import Bank D from Menu/Midi/Import patches from synth. It sends the correct Bank ID (0x02), but while importing it shows "importing Bank E". Same for when requesting Bank E. 0x03 gets send, import dialogue shows Bank F. Bank A works fine. Requesting Bank F doesn't do anything.
    Why is that?
    I "think" it just to work before I started using bankDescriptors in createBankDumpRequest

  2. Now when I select Bank D in the "In synth" tree, I get

00:53:27: error Adaptation[KawaiK5000]: Error calling createBankDumpRequest: ValueError: Invalid bank identifier '1'. Available banks: ['Bank A', 'Bank D', 'Bank E', 'Bank F']

At:
  C:\Users\Markus\KnobKraft-Adaptations\KawaiK5000.py(270): createBankDumpRequest

00:53:29: error Giving up retrying initiating bank dump

When I click on Bank E, it actually gets Bank D but fails with the foreign key error.
Why does that not honor the IDs from bankDescriptors or why is it different from getting Bank via Menu/Midi/Import patches from synth

Imho that's a bug in kk (different treatment of bank ID)?

@christofmuc
Copy link
Owner Author

Imho that's a bug in kk (different treatment of bank ID)?

Bank ID has to be 0, 1, 2, 3 for the C++ software, you can't have holes in there. KK will always call the first bank 0 and the second 1 - so the internal value is not the value that you would put into the request message.

Currently, because there is either bank B or bank DEF, you will get "bank 1" when it wants to request bank D.

@markusschloesser
Copy link
Collaborator

Imho that's a bug in kk (different treatment of bank ID)?

Bank ID has to be 0, 1, 2, 3 for the C++ software, you can't have holes in there. KK will always call the first bank 0 and the second 1 - so the internal value is not the value that you would put into the request message.

Currently, because there is either bank B or bank DEF, you will get "bank 1" when it wants to request bank D.

I was under the impression that bank descriptors are specifically for describing the banks of the synths and not internal kk banks? Also I don't understand the discrepancy between tree bank getting and menu bank getting.
Also, yeah the holes in the banks are a fucking mess 🙄

@markusschloesser
Copy link
Collaborator

oh and could you please check if I implemented the fingerprinting correctly? Also the build pipeline and automated testing fails for test_rename_should_not_change_fingerprint. Does that mean that my fingerprinting is wrong, or the rename function?

Expected :'97fdbd2fba69e9dd8eed504f1966b785'
Actual   :'1bc615041a13e9c27b97d74c1d498d19'
<Click to see difference>

adaptations\test_adaptations.py:146 (test_rename_should_not_change_fingerprint[KawaiK5000.py])
'1bc615041a13e9c27b97d74c1d498d19' != '97fdbd2fba69e9dd8eed504f1966b785'
adaptation = <module 'KawaiK5000.py' from 'KawaiK5000.py'>
test_data = TestData(sysex='testData/Kawai_K5000/full bank A midiOX K5000r.syx', program_generator=<function make_test_data.<local...n_convert_program_to_edit_buffer=True, can_convert_edit_buffer_to_program=True, rename_name=None, not_idempotent=False)
    @require_implemented("nameFromDump")
    @require_implemented("renamePatch")
    @require_implemented("calculateFingerprint")
    @require_testdata("programs")
    def test_rename_should_not_change_fingerprint(adaptation, test_data: testing.TestData):
        for program in test_data.programs:
            if program.dont_rename:
                continue
            binary = program.message.byte_list
            new_name = get_rename_target_name(program, test_data)
            renamed = adaptation.renamePatch(binary, new_name)
            # This should not change the fingerprint
>           assert adaptation.calculateFingerprint(renamed) == adaptation.calculateFingerprint(binary)
E           AssertionError: assert '1bc615041a13e9c27b97d74c1d498d19' == '97fdbd2fba69e9dd8eed504f1966b785'
E             - 97fdbd2fba69e9dd8eed504f1966b785
E             + 1bc615041a13e9c27b97d74c1d498d19
test_adaptations.py:159: AssertionError

The assumption is that the fingerprint stays the same after renaming, so fingerprinting should blank out the name before hashing, or not included the name bytes at all. Rename is tested separately and seems to work.

Thank you! Then I don't understand what I am doing wrong. I blank sysex header, the name and the checksum of the patch (because Imho renaming changes that)

…tually do not want the name to be part of the data that is submitted to the md5 function, because if you do, the md5 changes when the name changes
Copy link
Collaborator

@markusschloesser markusschloesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's exactly what I was trying to do ;-)

@christofmuc
Copy link
Owner Author

@markusschloesser Just FYI: I built a 2.6.2 which tries to fix the retry problem. Interested to hear if it works!

@markusschloesser
Copy link
Collaborator

with 2.6.2. and your changes I get

00:27:21: error Adaptation: Failure loading python module KawaiK5000: ImportError: cannot import name 'Simulator' from 'testing.test_data' (C:\Users\Markus\AppData\Local\Programs\KnobKraftOrm\testing\test_data.py)

At:
  C:\Users\Markus\KnobKraft-Adaptations\KawaiK5000.py(17): <module>
  <frozen importlib._bootstrap>(488): _call_with_frames_removed
  <frozen importlib._bootstrap_external>(995): exec_module
  <frozen importlib._bootstrap>(950): _load_unlocked
  <frozen importlib._bootstrap>(1333): _find_and_load_unlocked
  <frozen importlib._bootstrap>(1360): _find_and_load

00:27:21: error Unloading adaptation module C:\Users\Markus\KnobKraft-Adaptations\KawaiK5000.py

# Conflicts:
#	MidiKraft
#	adaptations/test_adaptations.py
@christofmuc
Copy link
Owner Author

with 2.6.2. and your changes I get

00:27:21: error Adaptation: Failure loading python module KawaiK5000: ImportError: cannot import name 'Simulator' from 'testing.test_data' (C:\Users\Markus\AppData\Local\Programs\KnobKraftOrm\testing\test_data.py)

At:
  C:\Users\Markus\KnobKraft-Adaptations\KawaiK5000.py(17): <module>
  <frozen importlib._bootstrap>(488): _call_with_frames_removed
  <frozen importlib._bootstrap_external>(995): exec_module
  <frozen importlib._bootstrap>(950): _load_unlocked
  <frozen importlib._bootstrap>(1333): _find_and_load_unlocked
  <frozen importlib._bootstrap>(1360): _find_and_load

00:27:21: error Unloading adaptation module C:\Users\Markus\KnobKraft-Adaptations\KawaiK5000.py

Yes, when you use the release it will not use the changed file adaptations/testing/test_data.py, which we need for the K5000, but the one it was shipped with the installer. You'd need to copy the file from the checked out merge request into the directory where you installed the Orm. Not sure how you did the setup before?

@markusschloesser
Copy link
Collaborator

with 2.6.2. and your changes I get

00:27:21: error Adaptation: Failure loading python module KawaiK5000: ImportError: cannot import name 'Simulator' from 'testing.test_data' (C:\Users\Markus\AppData\Local\Programs\KnobKraftOrm\testing\test_data.py)

At:
  C:\Users\Markus\KnobKraft-Adaptations\KawaiK5000.py(17): <module>
  <frozen importlib._bootstrap>(488): _call_with_frames_removed
  <frozen importlib._bootstrap_external>(995): exec_module
  <frozen importlib._bootstrap>(950): _load_unlocked
  <frozen importlib._bootstrap>(1333): _find_and_load_unlocked
  <frozen importlib._bootstrap>(1360): _find_and_load

00:27:21: error Unloading adaptation module C:\Users\Markus\KnobKraft-Adaptations\KawaiK5000.py

Yes, when you use the release it will not use the changed file adaptations/testing/test_data.py, which we need for the K5000, but the one it was shipped with the installer. You'd need to copy the file from the checked out merge request into the directory where you installed the Orm. Not sure how you did the setup before?

that worked!
never did this before. I thought that the testing part was only for running it in pycharm

@markusschloesser
Copy link
Collaborator

with 2.6.2. and your changes I get

00:27:21: error Adaptation: Failure loading python module KawaiK5000: ImportError: cannot import name 'Simulator' from 'testing.test_data' (C:\Users\Markus\AppData\Local\Programs\KnobKraftOrm\testing\test_data.py)

At:
  C:\Users\Markus\KnobKraft-Adaptations\KawaiK5000.py(17): <module>
  <frozen importlib._bootstrap>(488): _call_with_frames_removed
  <frozen importlib._bootstrap_external>(995): exec_module
  <frozen importlib._bootstrap>(950): _load_unlocked
  <frozen importlib._bootstrap>(1333): _find_and_load_unlocked
  <frozen importlib._bootstrap>(1360): _find_and_load

00:27:21: error Unloading adaptation module C:\Users\Markus\KnobKraft-Adaptations\KawaiK5000.py

Yes, when you use the release it will not use the changed file adaptations/testing/test_data.py, which we need for the K5000, but the one it was shipped with the installer. You'd need to copy the file from the checked out merge request into the directory where you installed the Orm. Not sure how you did the setup before?

that worked! never did this before. I thought that the testing part was only for running it in pycharm

but doesn't solve the retry problem, in fact made it worse. bank request doesn't get send/nothings happening in midi log, unless I cancel the request, then gets send 3 times

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants