Add Roland JD-800 - #538
Conversation
📝 WalkthroughWalkthroughThis PR adds support for the Roland JD-800 synthesizer by introducing a new adaptation module with complete program and bank management capabilities. The module is registered in the CMake build configuration and documented in the README's device compatibility table. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (2)
adaptations/Roland_JD800.py (2)
198-213: Missing blank line between top-level functions.PEP 8 expects two blank lines between top-level definitions; here
isSingleProgramDump(lines 211-212) is followed immediately bydef blankedOut(message):with no separator. Trivial, but if the project runsflake8/ruffwith E305/E302 enabled it will be flagged.♻️ Suggested fix
def isSingleProgramDump(messages): return jd_800.isSingleProgramDump(messages) + + def blankedOut(message):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adaptations/Roland_JD800.py` around lines 198 - 213, Add the required PEP8 separation: insert a blank line (so there are two blank lines between top-level definitions) between the end of isSingleProgramDump and the start of blankedOut; locate the functions isSingleProgramDump and blankedOut in the file and ensure there are two newline characters separating these top-level function definitions.
243-244:createBankDumpRequestignores its parameters and uses unexplained magic numbers.
channelandbankare accepted but never used. The hardcoded[0x05, 0x00, 0x00]duplicates_jd800_program_dump.base_address, and[0x01, 0x40, 0x00]is the 7-bit-encoded total payload size (96 × 256 = 24576 bytes) but expressed as opaque magic. If the JD-800 only ever has one bank, this is functionally correct, but a future caller passing a non-zerobankwill silently get bank 0. Consider either deriving these from the existing constants or asserting/raising on unexpectedbankvalues to make the single-bank assumption explicit.♻️ Suggested refactor
def createBankDumpRequest(channel, bank): - return jd_800.buildRolandMessage(jd_800.device_id, command_rq1, [0x05, 0x00, 0x00], [0x01, 0x40, 0x00]) + # JD-800 has a single internal patch bank; ignore `channel`/`bank`. + total_size = _BANK_PAGE_COUNT * _BANK_PAGE_SIZE # 24576 bytes + size_bytes = [ + (total_size >> 14) & 0x7F, + (total_size >> 7) & 0x7F, + total_size & 0x7F, + ] + return jd_800.buildRolandMessage( + jd_800.device_id, + command_rq1, + list(_jd800_program_dump.base_address), + size_bytes, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adaptations/Roland_JD800.py` around lines 243 - 244, The createBankDumpRequest function currently ignores its channel and bank parameters and uses hardcoded magic bytes; update it so the base address comes from the existing _jd800_program_dump.base_address (instead of [0x05,0x00,0x00]) and compute the 7-bit-encoded payload size from _jd800_program_dump.total_size (instead of [0x01,0x40,0x00]); if the device is single-bank, add an explicit assertion or raise if bank != 0 to make that assumption explicit; keep jd_800.device_id and command_rq1 for the jd_800.buildRolandMessage call and ensure channel is either used where required or validated likewise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@adaptations/Roland_JD800.py`:
- Around line 223-224: The MD5 usage in calculateFingerprint triggers Ruff S324;
add an inline suppression comment to the hashlib.md5 call to silence the warning
and document intent (e.g., "# noqa: S324 # MD5 used only for non-security
fingerprinting of blanked-out sysex payloads"). Update the line containing the
hashlib.md5(...) call in the calculateFingerprint function so the noqa
suppression and brief rationale are on the same line.
---
Nitpick comments:
In `@adaptations/Roland_JD800.py`:
- Around line 198-213: Add the required PEP8 separation: insert a blank line (so
there are two blank lines between top-level definitions) between the end of
isSingleProgramDump and the start of blankedOut; locate the functions
isSingleProgramDump and blankedOut in the file and ensure there are two newline
characters separating these top-level function definitions.
- Around line 243-244: The createBankDumpRequest function currently ignores its
channel and bank parameters and uses hardcoded magic bytes; update it so the
base address comes from the existing _jd800_program_dump.base_address (instead
of [0x05,0x00,0x00]) and compute the 7-bit-encoded payload size from
_jd800_program_dump.total_size (instead of [0x01,0x40,0x00]); if the device is
single-bank, add an explicit assertion or raise if bank != 0 to make that
assumption explicit; keep jd_800.device_id and command_rq1 for the
jd_800.buildRolandMessage call and ensure channel is either used where required
or validated likewise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5ba9c96-4900-43e1-9fc4-76af125e894c
📒 Files selected for processing (7)
README.mdadaptations/CMakeLists.txtadaptations/Roland_JD800.pyadaptations/testData/Roland_JD800/Bank-002-800.syxadaptations/testData/Roland_JD800/Internal_Demo_Patches.syxadaptations/testData/Roland_JD800/JDBOOK.SYXadaptations/testData/Roland_JD800/jd800_synthetic_bank.syx
| def calculateFingerprint(message): | ||
| return hashlib.md5(bytearray(blankedOut(message))).hexdigest() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm other adaptations use md5 for fingerprints (so behavior is consistent).
rg -nP '\bhashlib\.md5\s*\(' adaptationsRepository: christofmuc/KnobKraft-orm
Length of output: 4970
Add inline suppression for Ruff S324 hint on MD5 usage.
MD5 is used consistently across adaptations for non-security fingerprinting of blanked-out sysex payloads. Add the noqa comment to silence the Ruff warning and document intent:
Suggested fix
def calculateFingerprint(message):
- return hashlib.md5(bytearray(blankedOut(message))).hexdigest()
+ return hashlib.md5(bytearray(blankedOut(message))).hexdigest() # noqa: S324 - non-security fingerprint🧰 Tools
🪛 Ruff (0.15.11)
[error] 224-224: Probable use of insecure hash functions in hashlib: md5
(S324)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@adaptations/Roland_JD800.py` around lines 223 - 224, The MD5 usage in
calculateFingerprint triggers Ruff S324; add an inline suppression comment to
the hashlib.md5 call to silence the warning and document intent (e.g., "# noqa:
S324 # MD5 used only for non-security fingerprinting of blanked-out sysex
payloads"). Update the line containing the hashlib.md5(...) call in the
calculateFingerprint function so the noqa suppression and brief rationale are on
the same line.
|
Release-readiness follow-up (no fixes applied here):
23 existing tests pass on Python 3.12 against the current master harness. This is the base of the current #538 -> #539 -> #540 stack; please resolve the shared transport issue before advancing it. |
1 participant
Summary by CodeRabbit