docs: add ESP32/S3 musical-parameter mapping for encoder+TFT UI#58
docs: add ESP32/S3 musical-parameter mapping for encoder+TFT UI#58ovelhaaa wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA markdown document mapping ESP32-S3 audio DSP parameters to their implementation status in the hardware UI, categorizing them as: UI-exposed, core-implemented but UI-hidden, and DSP-available but unrouted, along with UX notes for potential interface restructuring. ChangesParameter Implementation Status Documentation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new documentation file mapping musical parameters for the ESP32/S3 interface, detailing their implementation status across the UI, bridge, and DSP core. The review identified several technical inconsistencies and improvement opportunities: a critical enum mismatch for ReadMode between the bridge and core, inconsistent default values for dcBlockEnabled, and potential calculation errors caused by UI-side BPM/Division processing instead of using core logic. Additionally, a clarification for the OFFSET parameter description was suggested to improve documentation accuracy.
| - **SMEAR / DIFFUSION AMOUNT** (`ap.smearAmount`) — fixo em `0.2f` na UI. | ||
| - **DIFFUSER STAGES** (`ap.diffuserStages`) — fixo em `2` na UI. | ||
| - **DC BLOCK ENABLE** (`ap.dcBlockEnabled`) — não exposto na UI; como `updateBridge()` cria `AudioParams` novo, as republicações da UI reimpõem o default (`true`). | ||
| - **READ MODE** (`ap.readMode`) — não exposto na UI; as republicações da UI também reimpõem o default (`Accidental`). |
There was a problem hiding this comment.
There is a critical mismatch between the ReadMode enum definitions in the bridge and the core. In parameter_bridge.h, Accidental is 0 and Orbit is 1. In orbit_delay_core.h, Orbit is 0 and AccidentalReverse is 1. Consequently, when the UI sends the default Accidental (0), the DSP core switches to Orbit (0). The documentation is correct about what the UI sends, but the system behavior is likely inverted relative to the labels.
| - **BPM** (`tempoBpm` local da UI) — implementado e editável, usado para calcular delay em amostras. | ||
| - **FEEDBACK** (`ap.feedback`) — implementado e editável. | ||
| - **DIVISION** (`noteDivision` local da UI) — implementado e editável, usado com BPM. | ||
| - **OFFSET** (`ap.offsetSamples`) — implementado de forma indireta via `tempoDelaySamples * OFFSET`. |
There was a problem hiding this comment.
The description of the OFFSET calculation is slightly ambiguous because it uses the term OFFSET both as the parameter name and as a factor in the formula (tempoDelaySamples * OFFSET). It would be clearer to specify that ap.offsetSamples is the resulting value sent to the bridge, calculated from a UI-side ratio. For example: - **OFFSET** (ap.offsetSamples) — implementado de forma indireta via tempoDelaySamples * ratio (onde ratio é o controle da UI).
| - **OUTPUT GAIN** (`ap.outputGain`) — não exposto na UI. | ||
| - **SMEAR / DIFFUSION AMOUNT** (`ap.smearAmount`) — fixo em `0.2f` na UI. | ||
| - **DIFFUSER STAGES** (`ap.diffuserStages`) — fixo em `2` na UI. | ||
| - **DC BLOCK ENABLE** (`ap.dcBlockEnabled`) — não exposto na UI; como `updateBridge()` cria `AudioParams` novo, as republicações da UI reimpõem o default (`true`). |
There was a problem hiding this comment.
This observation highlights a default value inconsistency. AudioParams in parameter_bridge.h defaults dcBlockEnabled to true, while OrbitDelayCore in orbit_delay_core.h defaults it to false. This causes the UI to override the DSP's intended default state upon the first publication. Aligning these defaults would ensure more predictable behavior.
| - **TEMPO BPM** (`setTempoBpm`) — existe no core, mas a app usa cálculo local de `offsetSamples`. | ||
| - **NOTE DIVISION** (`setNoteDivision`) — existe no core, mas a app usa cálculo local de `offsetSamples`. |
There was a problem hiding this comment.
The core already implements setTempoBpm and setNoteDivision with internal smoothing and sample-rate awareness. By calculating offsetSamples locally in the UI, the application bypasses this logic, which can lead to inconsistencies if the UI and Core use different sample rate assumptions or rounding. Consider routing BPM and Division directly to the core in the future UI rework.
1 participant
Motivation
Description
docs/esp32s3_parametros_interface.mdthat lists parameters and their implementation/exposure status for the ESP32/S3 build.Implementados e acessíveis pela UI,Implementados no bridge/core, mas não acessíveis na UI atual, andImplementados no core DSP, mas não roteados pelo AudioParamssections and document exact fields likeap.mix,ap.feedback,tempoBpm,ap.orbit, andsetShimmerMode.STEREO SPREAD,SMEAR,ORBIT,INPUT/OUTPUT GAIN,DIFFUSER STAGES,DC BLOCK, andSHIMMER MODE.Testing
Codex Task
Summary by CodeRabbit