E-mu Proteus#403
Conversation
|
These were my questions (minus the code pasted): We're doing some Python programming today! I will give you a file for a KnobKraft adaptation that is for the E-Mu Morpheus Synth. You will take that and change it so it supports the E-Mu Proteus Synth instead. If you need information about the Proteus, let me know, I can give it to you. Here is the adaptation code: First shot by DeepSeek Here is an editor implementaton in Java. Ignore all the stuff with UI construction and specific parameter changes. Try to extract the information that you need to make sure the Knobkraft adaptation works: Not too bad, but I think the name is not extracted correctly. Look again at the Java code, where did it extract the name of the patch from? It ended with an off by one error for number and name, because the "offset" of 7 in the Java code it transformed into an array access message[8:]. Very human like error. |
…e off by one errors itself when asked one by one to look at the implementation again.
|
Chat GPT is more cautios. Answer to first question: Got it! To adapt this script to the E-Mu Proteus, I'll need the Proteus-specific details, like: If you have a SysEx dump from the Proteus, that would also help in verifying the changes. It would not be able to read the whole Java file (too long input), but I gave it the parse() method I had find myself first, and then I could upload the factory bank sysex (!). That would suffice to create the adaptation. It still had questions about the banks though (me too). ChatGPT did remove my copyright header... which is interesting! No, it removed all comments actually from the input file. It made the same off by one error as DeepSeek, but asked was able to correct the file. I had to show it the error in convertToProgramDump additionally, then it worked. |
…yout. Starting a generic E-mu module here, but I do not really want to have to create one file per Synth, they have so many!
… to the GenericRoland module. This was copied into the D50 file for now so the user can test it without getting a new build.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request introduces a new E-mu emulation abstraction (GenericEmu), refactors RolandD50 into a class-based architecture (NonGenericRoland), adds device-ID auto-detection to Waldorf M, establishes a knobkraft API decorator framework, standardizes error handling across multiple adaptation modules, and updates README device status. The changes span infrastructure, device-specific logic, and error handling consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant Host as Host/Knobkraft
participant GenericEmu as GenericEmu Instance
participant Device as E-mu Device
rect rgb(200, 220, 240)
note over Host,Device: Device Detection & Version
Host->>GenericEmu: createDeviceDetectMessage(device_id)
GenericEmu->>Device: COMMAND_VERSION_REQUEST (sysex)
Device->>GenericEmu: COMMAND_VERSION_DATA (sysex)
GenericEmu->>Host: channelIfValidDeviceResponse(message)
end
rect rgb(200, 240, 200)
note over Host,Device: Program Dump Request & Parsing
Host->>GenericEmu: createProgramDumpRequest(device_id, patchNo)
GenericEmu->>Device: COMMAND_PRESET_REQUEST (sysex)
Device->>GenericEmu: COMMAND_PRESET_DATA (sysex)
GenericEmu->>Host: isSingleProgramDump(message)
GenericEmu->>Host: nameFromDump(message)
GenericEmu->>Host: numberFromDump(message)
end
rect rgb(240, 220, 200)
note over Host,Device: Patch Manipulation
Host->>GenericEmu: renamePatch(message, new_name)
GenericEmu->>Host: renamed_message
Host->>GenericEmu: calculateFingerprint(message)
GenericEmu->>Host: fingerprint_hash
end
sequenceDiagram
participant Host as Host/Knobkraft
participant Waldorf_M as Waldorf M Module
rect rgb(200, 220, 240)
note over Host,Waldorf_M: Device Detection with ID Capture
Host->>Waldorf_M: channelIfValidDeviceResponse(message)
Waldorf_M->>Waldorf_M: Extract & store device_id
Waldorf_M->>Waldorf_M: DEVICE_ID_DETECTED = extracted_id
Waldorf_M->>Host: channel (int)
end
rect rgb(200, 240, 200)
note over Host,Waldorf_M: Edit Buffer with Device-ID Awareness
Host->>Waldorf_M: createEditBufferRequest(channel)
Waldorf_M->>Waldorf_M: _device_id() → use DEVICE_ID_DETECTED
Waldorf_M->>Host: device-aware_sysex
Host->>Waldorf_M: convertToEditBuffer(channel, message)
Waldorf_M->>Waldorf_M: Inject detected device ID into payload
Waldorf_M->>Host: normalized_message
end
rect rgb(240, 220, 200)
note over Host,Waldorf_M: Bank Selection
Host->>Waldorf_M: bankSelect(channel, bank)
Waldorf_M->>Host: MIDI_CC_sysex
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
README.md(1 hunks)adaptations/Elektron_AnalogRytm.py(3 hunks)adaptations/Elektron_Digitone.py(3 hunks)adaptations/Emu_Proteus1.py(1 hunks)adaptations/Emu_Proteus1XR.py(1 hunks)adaptations/Moog_Voyager.py(4 hunks)adaptations/RolandD50.py(7 hunks)adaptations/Roland_JunoDS.py(1 hunks)adaptations/Waldorf_M.py(5 hunks)adaptations/emu/GenericEmu.py(1 hunks)adaptations/emu/__init__.py(1 hunks)adaptations/knobkraft/__init__.py(1 hunks)adaptations/roland/GenericRoland.py(3 hunks)adaptations/sequential/GenericSequential.py(1 hunks)adaptations/test_adaptations.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
adaptations/Moog_Voyager.py
79-79: Create your own exception
(TRY002)
79-79: Avoid specifying long messages outside the exception class
(TRY003)
153-153: Create your own exception
(TRY002)
153-153: Avoid specifying long messages outside the exception class
(TRY003)
162-162: Create your own exception
(TRY002)
162-162: Avoid specifying long messages outside the exception class
(TRY003)
180-180: Create your own exception
(TRY002)
180-180: Avoid specifying long messages outside the exception class
(TRY003)
adaptations/Elektron_Digitone.py
86-86: Create your own exception
(TRY002)
86-86: Avoid specifying long messages outside the exception class
(TRY003)
100-100: Create your own exception
(TRY002)
100-100: Avoid specifying long messages outside the exception class
(TRY003)
104-104: Create your own exception
(TRY002)
104-104: Avoid specifying long messages outside the exception class
(TRY003)
123-123: Create your own exception
(TRY002)
123-123: Avoid specifying long messages outside the exception class
(TRY003)
129-129: Create your own exception
(TRY002)
129-129: Avoid specifying long messages outside the exception class
(TRY003)
adaptations/sequential/GenericSequential.py
176-176: Create your own exception
(TRY002)
176-176: Avoid specifying long messages outside the exception class
(TRY003)
adaptations/emu/GenericEmu.py
58-58: Avoid specifying long messages outside the exception class
(TRY003)
90-90: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
91-91: Create your own exception
(TRY002)
91-91: Avoid specifying long messages outside the exception class
(TRY003)
98-98: Create your own exception
(TRY002)
98-98: Avoid specifying long messages outside the exception class
(TRY003)
105-105: Probable use of insecure hash functions in hashlib: md5
(S324)
106-106: Create your own exception
(TRY002)
106-106: Avoid specifying long messages outside the exception class
(TRY003)
adaptations/Roland_JunoDS.py
413-413: Create your own exception
(TRY002)
413-413: Avoid specifying long messages outside the exception class
(TRY003)
adaptations/Waldorf_M.py
28-28: Unused function argument: device_id
(ARG001)
58-58: Unused function argument: device_id
(ARG001)
71-71: Unused function argument: device_id
(ARG001)
80-80: Create your own exception
(TRY002)
80-80: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Unused function argument: device_id
(ARG001)
99-99: Unused function argument: device_id
(ARG001)
102-102: Consider [*message[0:3], _device_id(), *message[4:]] instead of concatenation
Replace with [*message[0:3], _device_id(), *message[4:]]
(RUF005)
108-108: Create your own exception
(TRY002)
108-108: Avoid specifying long messages outside the exception class
(TRY003)
119-119: Probable use of insecure hash functions in hashlib: md5
(S324)
120-120: Create your own exception
(TRY002)
120-120: Avoid specifying long messages outside the exception class
(TRY003)
adaptations/Elektron_AnalogRytm.py
107-107: Create your own exception
(TRY002)
107-107: Avoid specifying long messages outside the exception class
(TRY003)
132-132: Create your own exception
(TRY002)
132-132: Avoid specifying long messages outside the exception class
(TRY003)
136-136: Create your own exception
(TRY002)
136-136: Avoid specifying long messages outside the exception class
(TRY003)
153-153: Unpacked variable checksum is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
159-159: Create your own exception
(TRY002)
159-159: Avoid specifying long messages outside the exception class
(TRY003)
170-170: Create your own exception
(TRY002)
170-170: Avoid specifying long messages outside the exception class
(TRY003)
adaptations/RolandD50.py
53-53: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
53-53: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
53-53: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
208-208: Unpacked variable reply is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
233-233: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
246-246: Create your own exception
(TRY002)
246-246: Avoid specifying long messages outside the exception class
(TRY003)
247-247: Create your own exception
(TRY002)
247-247: Avoid specifying long messages outside the exception class
(TRY003)
263-263: Unused method argument: channel
(ARG002)
299-299: Unused method argument: channel
(ARG002)
304-304: Loop control variable message overrides iterable it iterates
(B020)
305-305: Unpacked variable command is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
305-305: Unpacked variable address is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
310-310: Create your own exception
(TRY002)
310-310: Avoid specifying long messages outside the exception class
(TRY003)
313-313: Unused method argument: channel
(ARG002)
350-350: Unused method argument: channel
(ARG002)
355-355: Loop control variable message overrides iterable it iterates
(B020)
361-361: Create your own exception
(TRY002)
361-361: Avoid specifying long messages outside the exception class
(TRY003)
384-384: Create your own exception
(TRY002)
384-384: Avoid specifying long messages outside the exception class
(TRY003)
388-388: Probable use of insecure hash functions in hashlib: md5
(S324)
449-449: Create your own exception
(TRY002)
449-449: Avoid specifying long messages outside the exception class
(TRY003)
484-484: Unpacked variable data is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
adaptations/roland/GenericRoland.py
100-100: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
100-100: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
100-100: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
408-408: Create your own exception
(TRY002)
408-408: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (5)
adaptations/emu/__init__.py (1)
1-5: Header addition acknowledgedLicensing header matches the project pattern; no issues spotted.
README.md (1)
80-80: Waldorf M status update looks goodGlad to see the device graduated to “works”; nothing to fix here.
adaptations/knobkraft/__init__.py (1)
10-12: Decorator centralization confirmedThe shared
knobkraft_apimarker is simple and correct; happy with this change.adaptations/test_adaptations.py (1)
332-333: Typo fix appreciatedThe comment now reads cleanly; no further action needed.
adaptations/roland/GenericRoland.py (1)
8-8: Roland typing/blanking changes look solidThe extended type hints and blank-out flexibility behave as expected; no issues to report.
| proteus1 = GenericEmu(name="E-mu Proteus/1", model_id=0x04, banks= [ | ||
| {"bank": 0, "name": "ROM", "size": 64, "type": "Preset", "isROM": True}, | ||
| {"bank": 1, "name": "RAM", "size": 64, "type": "Preset"}, | ||
| {"bank": 2, "name": "CARD", "size": 64, "type": "Preset", "isRom": True} | ||
| ]) |
There was a problem hiding this comment.
Use consistent ROM flag casing
Bank descriptors elsewhere rely on the isROM key. The CARD bank currently uses isRom, so it will be treated as writable even though the data is read-only. Please align the casing.
Apply this diff to fix it:
- {"bank": 2, "name": "CARD", "size": 64, "type": "Preset", "isRom": True}
+ {"bank": 2, "name": "CARD", "size": 64, "type": "Preset", "isROM": True}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| proteus1 = GenericEmu(name="E-mu Proteus/1", model_id=0x04, banks= [ | |
| {"bank": 0, "name": "ROM", "size": 64, "type": "Preset", "isROM": True}, | |
| {"bank": 1, "name": "RAM", "size": 64, "type": "Preset"}, | |
| {"bank": 2, "name": "CARD", "size": 64, "type": "Preset", "isRom": True} | |
| ]) | |
| proteus1 = GenericEmu(name="E-mu Proteus/1", model_id=0x04, banks= [ | |
| {"bank": 0, "name": "ROM", "size": 64, "type": "Preset", "isROM": True}, | |
| {"bank": 1, "name": "RAM", "size": 64, "type": "Preset"}, | |
| {"bank": 2, "name": "CARD", "size": 64, "type": "Preset", "isROM": True} | |
| ]) |
🤖 Prompt for AI Agents
In adaptations/Emu_Proteus1.py around lines 10 to 14, the CARD bank uses the
incorrect key casing "isRom" instead of the project-standard "isROM", causing it
to be treated as writable; update the CARD bank descriptor to use "isROM": True
to match the other bank entries and ensure the bank is recognized as read-only.
| proteus1xr = GenericEmu(name="E-mu Proteus/1 XR", model_id=0x07, banks=[ | ||
| {"bank": 0, "name": "RAM", "size": 64, "type": "Preset"}, | ||
| {"bank": 1, "name": "RAM", "size": 64, "type": "Preset"}, | ||
| {"bank": 2, "name": "RAM", "size": 64, "type": "Preset"}, | ||
| {"bank": 3, "name": "RAM", "size": 64, "type": "Preset"}, | ||
| {"bank": 4, "name": "ROM", "size": 64, "type": "Preset", "isRom": True}, | ||
| {"bank": 5, "name": "ROM", "size": 64, "type": "Preset", "isRom": True} |
There was a problem hiding this comment.
Fix ROM flag casing for XR banks
Same casing issue as the Proteus/1 module: the XR ROM banks use isRom, so downstream code will assume they’re writable. Please switch to isROM.
Suggested diff:
- {"bank": 4, "name": "ROM", "size": 64, "type": "Preset", "isRom": True},
- {"bank": 5, "name": "ROM", "size": 64, "type": "Preset", "isRom": True}
+ {"bank": 4, "name": "ROM", "size": 64, "type": "Preset", "isROM": True},
+ {"bank": 5, "name": "ROM", "size": 64, "type": "Preset", "isROM": True}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| proteus1xr = GenericEmu(name="E-mu Proteus/1 XR", model_id=0x07, banks=[ | |
| {"bank": 0, "name": "RAM", "size": 64, "type": "Preset"}, | |
| {"bank": 1, "name": "RAM", "size": 64, "type": "Preset"}, | |
| {"bank": 2, "name": "RAM", "size": 64, "type": "Preset"}, | |
| {"bank": 3, "name": "RAM", "size": 64, "type": "Preset"}, | |
| {"bank": 4, "name": "ROM", "size": 64, "type": "Preset", "isRom": True}, | |
| {"bank": 5, "name": "ROM", "size": 64, "type": "Preset", "isRom": True} | |
| proteus1xr = GenericEmu(name="E-mu Proteus/1 XR", model_id=0x07, banks=[ | |
| {"bank": 0, "name": "RAM", "size": 64, "type": "Preset"}, | |
| {"bank": 1, "name": "RAM", "size": 64, "type": "Preset"}, | |
| {"bank": 2, "name": "RAM", "size": 64, "type": "Preset"}, | |
| {"bank": 3, "name": "RAM", "size": 64, "type": "Preset"}, | |
| {"bank": 4, "name": "ROM", "size": 64, "type": "Preset", "isROM": True}, | |
| {"bank": 5, "name": "ROM", "size": 64, "type": "Preset", "isROM": True} |
🤖 Prompt for AI Agents
In adaptations/Emu_Proteus1XR.py around lines 10 to 16, the ROM banks use the
wrong casing for the ROM flag ("isRom") so downstream logic treats them as
writable; change those bank dict entries to use "isROM" instead of "isRom" for
the ROM banks (lines defining bank 4 and bank 5) so the flag matches the
expected key and downstream code recognizes them as read-only.
| def createProgramDumpRequest(self, device_id, patchNo): | ||
| if not 0 <= patchNo < 320: | ||
| raise ValueError(f"Patch number out of range: {patchNo} should be from 0 to 319") | ||
| return [[0xf0, EMU_ID, model_id, device_id & 0x0f, COMMAND_PRESET_REQUEST, patchNo & 0x7f, (patchNo >> 7) & 0x7f, 0xf7] for model_id in PROTEUS_IDS] | ||
|
|
There was a problem hiding this comment.
Return only the active model’s request
createProgramDumpRequest now emits seven requests (one per Proteus ID). Callers expect a single SysEx packet for the selected device, and the new list-of-lists return value breaks both the test suite (test_program_dump_request) and runtime scheduling.
Please emit only this model’s request:
- return [[0xf0, EMU_ID, model_id, device_id & 0x0f, COMMAND_PRESET_REQUEST, patchNo & 0x7f, (patchNo >> 7) & 0x7f, 0xf7] for model_id in PROTEUS_IDS]
+ return [0xf0, EMU_ID, self._model_id, device_id & 0x0f, COMMAND_PRESET_REQUEST, patchNo & 0x7f, (patchNo >> 7) & 0x7f, 0xf7]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def createProgramDumpRequest(self, device_id, patchNo): | |
| if not 0 <= patchNo < 320: | |
| raise ValueError(f"Patch number out of range: {patchNo} should be from 0 to 319") | |
| return [[0xf0, EMU_ID, model_id, device_id & 0x0f, COMMAND_PRESET_REQUEST, patchNo & 0x7f, (patchNo >> 7) & 0x7f, 0xf7] for model_id in PROTEUS_IDS] | |
| def createProgramDumpRequest(self, device_id, patchNo): | |
| if not 0 <= patchNo < 320: | |
| raise ValueError(f"Patch number out of range: {patchNo} should be from 0 to 319") | |
| return [0xf0, EMU_ID, self._model_id, device_id & 0x0f, COMMAND_PRESET_REQUEST, patchNo & 0x7f, (patchNo >> 7) & 0x7f, 0xf7] |
🧰 Tools
🪛 Ruff (0.14.2)
58-58: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In adaptations/emu/GenericEmu.py around lines 56 to 60, the function currently
returns a list of SysEx requests (one per PROTEUS_IDS) but callers expect a
single SysEx packet for the active model; replace the list comprehension with
code that selects a single model_id and returns only that request. Compute an
index from the device_id high nibble (e.g. (device_id >> 4) & 0x0f) and map it
into PROTEUS_IDS via modulo to pick the active model_id, then build and return a
single list [0xf0, EMU_ID, model_id, device_id & 0x0f, COMMAND_PRESET_REQUEST,
patchNo & 0x7f, (patchNo >> 7) & 0x7f, 0xf7].
| def renamePatch(self, message: List[int], new_name: str) -> List[int]: | ||
| if self.isSingleProgramDump(message): | ||
| name_params = [(ord(c) & 0x7f, (ord(c) >> 7) & 0x7f) for c in new_name.ljust(12, " ")] | ||
| return message[:7] + [item for sublist in name_params for item in sublist] + message[31:] |
There was a problem hiding this comment.
Clamp rename payload to 12 characters
new_name.ljust(12, " ") passes through longer names unchanged, so a 16-character name inserts 32 bytes and corrupts the SysEx structure. Truncate before padding so we always write exactly 12 characters.
Proposed fix:
- name_params = [(ord(c) & 0x7f, (ord(c) >> 7) & 0x7f) for c in new_name.ljust(12, " ")]
+ name_params = [(ord(c) & 0x7f, (ord(c) >> 7) & 0x7f) for c in new_name[:12].ljust(12, " ")]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def renamePatch(self, message: List[int], new_name: str) -> List[int]: | |
| if self.isSingleProgramDump(message): | |
| name_params = [(ord(c) & 0x7f, (ord(c) >> 7) & 0x7f) for c in new_name.ljust(12, " ")] | |
| return message[:7] + [item for sublist in name_params for item in sublist] + message[31:] | |
| def renamePatch(self, message: List[int], new_name: str) -> List[int]: | |
| if self.isSingleProgramDump(message): | |
| name_params = [(ord(c) & 0x7f, (ord(c) >> 7) & 0x7f) for c in new_name[:12].ljust(12, " ")] | |
| return message[:7] + [item for sublist in name_params for item in sublist] + message[31:] |
🤖 Prompt for AI Agents
In adaptations/emu/GenericEmu.py around lines 94 to 97, the renamePatch function
pads new_name to 12 chars but doesn't truncate longer names, allowing >12 chars
to expand the payload and corrupt the SysEx; change the code to first truncate
new_name to 12 characters (e.g., new_name[:12]) and then left-pad it to 12
(ljust) before encoding into the 2-byte-per-char name_params so the function
always writes exactly 12 characters (24 bytes) into the message.
| if isOwnSysex(message): | ||
| first_message = knobkraft.findSysexDelimiters(message, 1)[0] | ||
| checksum_start = first_message[0] + self._checksum_start() | ||
| checksum = self.roland_checksum(message[checksum_start:first_message[1]-2]) | ||
| if checksum == message[first_message[1]-2]: | ||
| command = message[first_message[0]+3 + self._model_id_len] | ||
| address = message[checksum_start:checksum_start + self.address_size] | ||
| return command, address, message[checksum_start + self.address_size:-2] | ||
| raise Exception("Checksum error in Roland message parsing, expected", message[-2], "but got", checksum) | ||
| raise Exception("Failed to identify this as a D-50 message") |
There was a problem hiding this comment.
Fix parseRolandMessage to respect the instance model ID.
parseRolandMessage still calls the global isOwnSysex, which hard-codes the D-50 model ID. Any new NonGenericRoland instance (e.g., the Proteus work in this PR) will fail to parse its own SysEx and always throw “Failed to identify this as a D-50 message.” Use the instance method so detection keys off the instance’s model_id.
Apply this diff:
- if isOwnSysex(message):
+ if self.isOwnSysex(message):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if isOwnSysex(message): | |
| first_message = knobkraft.findSysexDelimiters(message, 1)[0] | |
| checksum_start = first_message[0] + self._checksum_start() | |
| checksum = self.roland_checksum(message[checksum_start:first_message[1]-2]) | |
| if checksum == message[first_message[1]-2]: | |
| command = message[first_message[0]+3 + self._model_id_len] | |
| address = message[checksum_start:checksum_start + self.address_size] | |
| return command, address, message[checksum_start + self.address_size:-2] | |
| raise Exception("Checksum error in Roland message parsing, expected", message[-2], "but got", checksum) | |
| raise Exception("Failed to identify this as a D-50 message") | |
| if self.isOwnSysex(message): | |
| first_message = knobkraft.findSysexDelimiters(message, 1)[0] | |
| checksum_start = first_message[0] + self._checksum_start() | |
| checksum = self.roland_checksum(message[checksum_start:first_message[1]-2]) | |
| if checksum == message[first_message[1]-2]: | |
| command = message[first_message[0]+3 + self._model_id_len] | |
| address = message[checksum_start:checksum_start + self.address_size] | |
| return command, address, message[checksum_start + self.address_size:-2] | |
| raise Exception("Checksum error in Roland message parsing, expected", message[-2], "but got", checksum) | |
| raise Exception("Failed to identify this as a D-50 message") |
🧰 Tools
🪛 Ruff (0.14.2)
246-246: Create your own exception
(TRY002)
246-246: Avoid specifying long messages outside the exception class
(TRY003)
247-247: Create your own exception
(TRY002)
247-247: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In adaptations/RolandD50.py around lines 238 to 247, parseRolandMessage
currently calls the global isOwnSysex which uses a hard-coded D-50 model id;
change it to call the instance method (self.isOwnSysex(message)) so detection
uses the instance's model_id, then leave the rest of the parsing logic unchanged
(compute first_message, checksum_start, checksum, compare, extract
command/address/data and raise the same exceptions on checksum or identification
failure).
| def changeAddressInRolandMessage(self, message: List[int], new_address: List[int]) -> List[int]: | ||
| checksum_start = self._checksum_start() | ||
| assert len(new_address) == self.address_size | ||
| return message[:checksum_start] + new_address + message[checksum_start + self.address_size:] | ||
|
|
There was a problem hiding this comment.
Recompute the checksum after rewriting the address.
Changing the address alters the payload that feeds the Roland checksum, so returning the spliced list without updating message[-2] produces an invalid SysEx frame that the synth will reject. Recalculate the checksum before returning.
Apply this diff:
- return message[:checksum_start] + new_address + message[checksum_start + self.address_size:]
+ new_message = message[:checksum_start] + new_address + message[checksum_start + self.address_size:]
+ new_message[-2] = self.roland_checksum(new_message[checksum_start:-2])
+ return new_message📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def changeAddressInRolandMessage(self, message: List[int], new_address: List[int]) -> List[int]: | |
| checksum_start = self._checksum_start() | |
| assert len(new_address) == self.address_size | |
| return message[:checksum_start] + new_address + message[checksum_start + self.address_size:] | |
| def changeAddressInRolandMessage(self, message: List[int], new_address: List[int]) -> List[int]: | |
| checksum_start = self._checksum_start() | |
| assert len(new_address) == self.address_size | |
| new_message = message[:checksum_start] + new_address + message[checksum_start + self.address_size:] | |
| new_message[-2] = self.roland_checksum(new_message[checksum_start:-2]) | |
| return new_message |
🤖 Prompt for AI Agents
In adaptations/RolandD50.py around lines 253 to 257, after splicing in the
new_address you must recompute the Roland SysEx checksum and write it back (the
review notes it lives at message[-2]). Compute the checksum over the same
payload range the Roland algorithm expects (use self._checksum_start() /
checksum_start and the bytes up to the checksum byte), set message[-2] (or the
appropriate checksum index computed from checksum_start and address_size) to the
new checksum value, then return the modified message so the frame is valid.
| blanked_out = copy(message) | ||
| blanked_out[32:34] = [0, 0] | ||
| blanked_out[32:34] = [0, 0] | ||
| blanked_out[32:35] = [0, 0] | ||
| blanked_out[32:35] = [0, 0] | ||
| blanked_out[6:6+23] = [0] * 23 | ||
| blanked_out[3] = 0x00 | ||
| return hashlib.md5(bytearray(blanked_out)).hexdigest() # Calculate the fingerprint from the cleaned payload data |
There was a problem hiding this comment.
Restore zeroed program bytes without shrinking the message.
blanked_out[32:35] = [0, 0] assigns only two elements to a 3-byte slice, so the list loses one byte (and the second assignment keeps it short). That corrupts the payload used for fingerprinting by dropping the final byte of the original slice. Replace both assignments with a single three-byte fill so the message length stays at 512.
- blanked_out[32:35] = [0, 0]
- blanked_out[32:35] = [0, 0]
+ blanked_out[32:35] = [0, 0, 0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| blanked_out = copy(message) | |
| blanked_out[32:34] = [0, 0] | |
| blanked_out[32:34] = [0, 0] | |
| blanked_out[32:35] = [0, 0] | |
| blanked_out[32:35] = [0, 0] | |
| blanked_out[6:6+23] = [0] * 23 | |
| blanked_out[3] = 0x00 | |
| return hashlib.md5(bytearray(blanked_out)).hexdigest() # Calculate the fingerprint from the cleaned payload data | |
| blanked_out = copy(message) | |
| blanked_out[32:35] = [0, 0, 0] | |
| blanked_out[6:6+23] = [0] * 23 | |
| blanked_out[3] = 0x00 | |
| return hashlib.md5(bytearray(blanked_out)).hexdigest() # Calculate the fingerprint from the cleaned payload data |
🧰 Tools
🪛 Ruff (0.14.2)
119-119: Probable use of insecure hash functions in hashlib: md5
(S324)
🤖 Prompt for AI Agents
In adaptations/Waldorf_M.py around lines 114 to 119, the two assignments
blanked_out[32:35] = [0, 0] (done twice) only supply two bytes to a three-byte
slice and thus shrinks the message by one byte; replace both with a single
three-byte assignment (e.g., blanked_out[32:35] = [0, 0, 0]) so the slice is
fully zeroed without changing the overall message length (keep final message
length 512) before computing the MD5 fingerprint.
1 participant
By DeepSeek and me.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation