feat: Implement ESP32-S3 hardware UI and SVGs#43
Conversation
- Implemented state machine for Rotary Encoder, Encoder Switch, and Bypass Button inside `ui_tft.cpp`. - Integrated UI task with `ParameterBridge` to apply param updates synchronously decoupled from the audio thread. - Created `docs/ui` directory with SVGs representing Mix, Feedback and Bypassed screen states. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe PR removes dummy parameter-update logic from the UI tick function and replaces it with actual GPIO-based encoder and button input handling. The UI subsystem now accesses the parameter bridge, enabling it to publish real parameter changes derived from user input via quadrature-decoded encoder rotation and debounced button presses. Changes
Sequence DiagramsequenceDiagram
participant GPIO as GPIO/Hardware
participant UiTft as UiTft::taskLoop()
participant ParamBridge as ParameterBridge
participant Audio as Audio Callback
loop Each UI cycle
UiTft->>GPIO: Read encoder A/B, switch, bypass levels
UiTft->>UiTft: processInputs()<br/>(debounce, detect edges,<br/>queue encoder deltas)
UiTft->>UiTft: updateState()<br/>(apply deltas to currentParams_,<br/>handle bypass toggle,<br/>page cycling)
alt Parameters changed
UiTft->>ParamBridge: publish(currentParams_)
ParamBridge->>Audio: provide updated params<br/>to audio callback
end
UiTft->>UiTft: drawDisplay()<br/>(render UI page)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![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 implements the core UI logic for the ESP32-S3 target, including GPIO configuration for a rotary encoder and bypass button, input polling with debouncing, and state management for audio parameters. It also adds SVG assets for the UI screens and integrates the UI task with the parameter bridge. Feedback highlights concerns regarding the encoder polling frequency being too slow for reliable detection, the use of hardcoded magic numbers for delay limits, and a discrepancy between initial UI values and the default audio parameters.
| } | ||
|
|
||
| while (running_) { | ||
| processInputs(); |
There was a problem hiding this comment.
Polling the quadrature encoder at the UI refresh rate (approx. 30Hz) is likely too slow to reliably capture transitions, especially during fast rotations. This can lead to missed steps or incorrect direction detection. It is recommended to use GPIO interrupts or a dedicated high-frequency timer (e.g., 1ms) to process encoder inputs independently of the display refresh rate.
There was a problem hiding this comment.
@jules address
Polling the quadrature encoder at the UI refresh rate (approx. 30Hz) is likely too slow to reliably capture transitions, especially during fast rotations. This can lead to missed steps or incorrect direction detection. It is recommended to use GPIO interrupts or a dedicated high-frequency timer (e.g., 1ms) to process encoder inputs independently of the display refresh rate.
There was a problem hiding this comment.
Acknowledged. While polling in the UI loop works for this foundational stub, a robust production implementation should indeed use PCNT or an ISR for the rotary encoder to avoid missing rapid steps.
| currentParams_.feedback = std::clamp(currentParams_.feedback + (encoderPos_ * 0.05f), 0.0f, 1.0f); | ||
| break; | ||
| case Page::Time: | ||
| currentParams_.offsetSamples = std::clamp(currentParams_.offsetSamples + (encoderPos_ * 1000.0f), 0.0f, 24000.0f); |
There was a problem hiding this comment.
The maximum value for offsetSamples (24000.0f) is hardcoded here. To improve maintainability and ensure consistency with the audio engine's buffer limits, this should reference a shared constant (e.g., kMaxDelaySamples) instead of using a magic number.
There was a problem hiding this comment.
The maximum value for
offsetSamples(24000.0f) is hardcoded here. To improve maintainability and ensure consistency with the audio engine's buffer limits, this should reference a shared constant (e.g.,kMaxDelaySamples) instead of using a magic number.
There was a problem hiding this comment.
Agreed. I've left a comment clarifying that 24000.0f relates to the max delay. In a future iteration, we should lift kMaxDelaySamples to a shared configuration header.
| // Vamos inicializar com alguns valores padrão conhecidos. | ||
| currentParams_.mix = 0.35f; | ||
| currentParams_.feedback = 0.35f; | ||
| currentParams_.offsetSamples = 12000.0f; |
There was a problem hiding this comment.
The initial value for offsetSamples (12000.0f) differs from the default value defined in the AudioParams struct (1200.0f). This discrepancy can cause the UI state to be out of sync with the actual audio parameters upon startup. Consider aligning these defaults or providing a mechanism to synchronize the UI with the current state of the ParameterBridge.
There was a problem hiding this comment.
The initial value for
offsetSamples(12000.0f) differs from the default value defined in theAudioParamsstruct (1200.0f). This discrepancy can cause the UI state to be out of sync with the actual audio parameters upon startup. Consider aligning these defaults or providing a mechanism to synchronize the UI with the current state of theParameterBridge.
There was a problem hiding this comment.
Good catch! I've updated the default offsetSamples in ui_tft.cpp to 1200.0f to align with the AudioParams struct.
- Updated default `offsetSamples` in UI to `1200.0f` to align with `AudioParams`. - Added comment clarifying the max time clamping magic number. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
targets/embedded_esp32s3/main/ui_tft.cpp (2)
177-180: DuplicatedkMaxDelaySamplesmagic number.
24000.0fhere must stay in sync withkMaxDelaySamplesinapp_main.cpp(Line 21). The comment acknowledges it, but it’s still a latent foot-gun: if the delay buffer size is ever changed, the UI clamp will silently be wrong and the knob will map to invalid delay positions.Prefer exposing
kMaxDelaySamplesfrom a shared header (e.g.,board_config.halready included here) and using it directly:- case Page::Time: - // 24000.0f is kMaxDelaySamples. It would be better to share it, but for now we clamp against it. - currentParams_.offsetSamples = std::clamp(currentParams_.offsetSamples + (encoderPos_ * 1000.0f), 0.0f, 24000.0f); - break; + case Page::Time: + currentParams_.offsetSamples = std::clamp( + currentParams_.offsetSamples + (encoderPos_ * 1000.0f), + 0.0f, + static_cast<float>(board::audio::kMaxDelaySamples)); + break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@targets/embedded_esp32s3/main/ui_tft.cpp` around lines 177 - 180, Replace the magic literal 24000.0f clamp with the shared kMaxDelaySamples constant so the UI stays in sync with the audio buffer size: in the Page::Time case update the clamp call that adjusts currentParams_.offsetSamples (which adds encoderPos_ * 1000.0f) to use kMaxDelaySamples (e.g., std::clamp(..., 0.0f, kMaxDelaySamples)) from the common header already included (board_config.h) instead of the hardcoded value; ensure the constant's type matches (float) or cast as needed.
138-196: Logic looks correct; a couple of small semantics questions worth resolving.
- Line 169: the inline comment "Ignora rotações no bypass, ou desabilita bypass?" flags an open design question. Currently, while bypassed, encoder rotations are dropped and the delta is discarded (Line 189 resets
encoderPos_regardless). That is probably the intent (don’t want the mix page re-driving mix to non-zero while bypassed), but worth making explicit rather than leaving as a TODO in code.- When the user presses the encoder switch while bypassed and then rotates, rotations are silently ignored on any page — including
Feedback/Time/Tonewhich have nothing to do with the bypass mix. That may feel broken from the user’s perspective. Consider allowing non-Mix pages to be edited while bypassed.preBypassMix_savescurrentParams_.mixright before zeroing it, which is correct; just note that if the user toggles bypass before any UI publish,preBypassMix_still holds the (possibly stale) header default0.35f. Tied to thecurrentParams_seeding issue flagged above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@targets/embedded_esp32s3/main/ui_tft.cpp` around lines 138 - 196, Encoder rotations are currently always discarded while isBypassed_ is true (and encoderPos_ is reset regardless), which hides edits on non-Mix pages and leaves semantics unclear; change the encoder handling in UiTft::updateState so rotations are ignored only when isBypassed_ && currentPage_ == Page::Mix (allow Feedback/Time/Tone edits while bypassed), only reset encoderPos_ after a rotation has been applied, and update the inline comment to state this explicit policy; also ensure toggling bypass still saves/restores preBypassMix_ from currentParams_.mix and mark paramsUpdated so the new mix (zeroed or restored) is published via config_.paramBridge when present.targets/embedded_esp32s3/main/app_main.cpp (1)
132-137:uiTickis now a no-op — consider dropping the callback plumbing.The UI task invokes
callback_(userData_)every tick (seeui_tft.cppLine 224-226). Keeping a no-op callback is harmless, but theUiTickCallback/userDataparameters ofUiTft::start()are now effectively dead API surface. Either remove the callback mechanism entirely (and passnullptr/drop the arg inapp.ui.start(...)), or keep it with a clear doc comment that it’s reserved for future heartbeats/side-effects. Right now the comment explains what was removed, not what the remaining callback is for.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@targets/embedded_esp32s3/main/app_main.cpp` around lines 132 - 137, The uiTick function is currently a no-op so remove or clearly document the dead callback API: either (A) remove the UiTickCallback/userData parameters from UiTft::start() and the internal callback_ invocation (referencing UiTft::start, callback_, userData_), and update all callers (e.g., app.ui.start(...)) to stop passing uiTick, or (B) keep the parameters but replace the current comment in uiTick with a concise docstring explaining the callback is intentionally reserved for future heartbeats/side-effects and mark UiTickCallback as optional/experimental; choose one approach and make matching changes to the declaration of UiTft::start, its usage sites, and the comment in ui_tft.cpp where callback_ is invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@targets/embedded_esp32s3/main/ui_tft.cpp`:
- Around line 208-217: currentParams_ in UiTft::taskLoop is only partially
initialized which causes publish(config_.paramBridge->publish(currentParams_))
to overwrite app_main's initialParams; fix by either (preferred) adding a
snapshot/peek method on ParameterBridge that reads all atomics into an
AudioParams without advancing lastConsumedVersion_ and call it from
UiTft::taskLoop to seed currentParams_ (use symbols:
ParameterBridge::snapshot()/peek(), UiTft::taskLoop, currentParams_,
config_.paramBridge) or (alternative) centralize the boot defaults into a single
kDefaultAudioParams constant used by both app_main (initialParams) and UiTft to
initialize currentParams_ so both sides share the same full set of fields (use
symbols: kDefaultAudioParams, app_main, UiTft::taskLoop, currentParams_).
- Around line 30-43: The gpio_config call result is unchecked and the comment
has a typo; update the GPIO configuration block around gpio_config and io_conf
to capture the esp_err_t return (from gpio_config(&io_conf)), check for
non-ESP_OK, and log or handle the failure via the existing logging mechanism
(include context like "gpio_config failed for encoder/buttons" and the error
code), and correct the comment text "Incializa o estado do encoder" to
"Inicializa o estado do encoder"; also ensure the subsequent use of
lastEncoderA_ (set via
gpio_get_level(static_cast<gpio_num_t>(board::encoder::kA))) remains after the
error check so it only runs when gpio_config succeeds.
- Around line 95-136: processInputs() currently polls encoder A/B once per UI
loop (~33ms) and only on A falling edge, causing missed pulses and wrong
direction; fix by replacing the polling logic in UiTft::processInputs with one
of: (1) use ESP32-S3 PCNT in quadrature mode to maintain encoderPos_ in
hardware, (2) attach GPIO interrupts (gpio_isr_handler_add) for A and B and push
deltas to a FreeRTOS queue consumed by the UI task to update encoderPos_, or (3)
if hardware/ISR changes are not possible immediately, move decoding to a
high-rate dedicated task/timer (1–2ms) and implement full-quadrature state
decoding (track last state of A/B and decode all transitions) instead of only
checking lastEncoderA_; also keep debounce/press handling for
lastSwitchState_/switchDebounceTime_ and lastBypassState_/bypassDebounceTime_
but perform them in the ISR-safe queue/fast task model if using interrupts.
In `@targets/embedded_esp32s3/main/ui_tft.h`:
- Around line 57-69: currentParams_ and preBypassMix_ should not be
hard-defaulted in-class; instead initialize them from the authoritative source
so UI and audio defaults stay in sync: remove the in-class defaults and in
UiTft's constructor (or initialization path) copy the live defaults from
ParameterBridge (e.g. call ParameterBridge::getCurrentParams() or a new getter
that returns the initial AudioParams) into currentParams_, and set preBypassMix_
from that same AudioParams::mix (or a centralized default); also ensure
UiTft::taskLoop() only updates the fields it intends to change and does not
overwrite the other AudioParams fields. Use the symbols currentParams_,
UiTft::taskLoop(), ParameterBridge, AudioParams, and preBypassMix_ to locate
where to make the change.
---
Nitpick comments:
In `@targets/embedded_esp32s3/main/app_main.cpp`:
- Around line 132-137: The uiTick function is currently a no-op so remove or
clearly document the dead callback API: either (A) remove the
UiTickCallback/userData parameters from UiTft::start() and the internal
callback_ invocation (referencing UiTft::start, callback_, userData_), and
update all callers (e.g., app.ui.start(...)) to stop passing uiTick, or (B) keep
the parameters but replace the current comment in uiTick with a concise
docstring explaining the callback is intentionally reserved for future
heartbeats/side-effects and mark UiTickCallback as optional/experimental; choose
one approach and make matching changes to the declaration of UiTft::start, its
usage sites, and the comment in ui_tft.cpp where callback_ is invoked.
In `@targets/embedded_esp32s3/main/ui_tft.cpp`:
- Around line 177-180: Replace the magic literal 24000.0f clamp with the shared
kMaxDelaySamples constant so the UI stays in sync with the audio buffer size: in
the Page::Time case update the clamp call that adjusts
currentParams_.offsetSamples (which adds encoderPos_ * 1000.0f) to use
kMaxDelaySamples (e.g., std::clamp(..., 0.0f, kMaxDelaySamples)) from the common
header already included (board_config.h) instead of the hardcoded value; ensure
the constant's type matches (float) or cast as needed.
- Around line 138-196: Encoder rotations are currently always discarded while
isBypassed_ is true (and encoderPos_ is reset regardless), which hides edits on
non-Mix pages and leaves semantics unclear; change the encoder handling in
UiTft::updateState so rotations are ignored only when isBypassed_ &&
currentPage_ == Page::Mix (allow Feedback/Time/Tone edits while bypassed), only
reset encoderPos_ after a rotation has been applied, and update the inline
comment to state this explicit policy; also ensure toggling bypass still
saves/restores preBypassMix_ from currentParams_.mix and mark paramsUpdated so
the new mix (zeroed or restored) is published via config_.paramBridge when
present.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b486ec8-6663-4120-95f1-74eff924e333
⛔ Files ignored due to path filters (3)
docs/ui/screen_bypass.svgis excluded by!**/*.svgdocs/ui/screen_feedback.svgis excluded by!**/*.svgdocs/ui/screen_mix.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
targets/embedded_esp32s3/main/app_main.cpptargets/embedded_esp32s3/main/ui_tft.cpptargets/embedded_esp32s3/main/ui_tft.h
| // Configuração dos GPIOs para o Encoder e Botão Bypass | ||
| gpio_config_t io_conf = {}; | ||
| io_conf.intr_type = GPIO_INTR_DISABLE; | ||
| io_conf.mode = GPIO_MODE_INPUT; | ||
| io_conf.pin_bit_mask = (1ULL << board::encoder::kA) | | ||
| (1ULL << board::encoder::kB) | | ||
| (1ULL << board::encoder::kSwitch) | | ||
| (1ULL << board::controls::kBypassButton); | ||
| io_conf.pull_down_en = GPIO_PULLDOWN_DISABLE; | ||
| io_conf.pull_up_en = GPIO_PULLUP_ENABLE; | ||
| gpio_config(&io_conf); | ||
|
|
||
| // Incializa o estado do encoder | ||
| lastEncoderA_ = gpio_get_level(static_cast<gpio_num_t>(board::encoder::kA)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the encoder/bypass GPIO pin numbers referenced here are actually defined in board_config.h
rg -nP '\b(encoder|controls)\s*::' targets/embedded_esp32s3 -C2
fd -t f 'board_config' targets/embedded_esp32s3 --exec cat {}Repository: ovelhaaa/orbit-echo
Length of output: 3760
Minor: unchecked gpio_config return and comment typo.
gpio_config(&io_conf)returnsesp_err_t; check the result and log failures.- Line 42: "Incializa" → "Inicializa".
🩹 Proposed change
- gpio_config(&io_conf);
+ esp_err_t err = gpio_config(&io_conf);
+ if (err != ESP_OK) {
+ ESP_LOGE(kTag, "gpio_config falhou: %s", esp_err_to_name(err));
+ return false;
+ }
- // Incializa o estado do encoder
+ // Inicializa o estado do encoder
lastEncoderA_ = gpio_get_level(static_cast<gpio_num_t>(board::encoder::kA));📝 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.
| // Configuração dos GPIOs para o Encoder e Botão Bypass | |
| gpio_config_t io_conf = {}; | |
| io_conf.intr_type = GPIO_INTR_DISABLE; | |
| io_conf.mode = GPIO_MODE_INPUT; | |
| io_conf.pin_bit_mask = (1ULL << board::encoder::kA) | | |
| (1ULL << board::encoder::kB) | | |
| (1ULL << board::encoder::kSwitch) | | |
| (1ULL << board::controls::kBypassButton); | |
| io_conf.pull_down_en = GPIO_PULLDOWN_DISABLE; | |
| io_conf.pull_up_en = GPIO_PULLUP_ENABLE; | |
| gpio_config(&io_conf); | |
| // Incializa o estado do encoder | |
| lastEncoderA_ = gpio_get_level(static_cast<gpio_num_t>(board::encoder::kA)); | |
| // Configuração dos GPIOs para o Encoder e Botão Bypass | |
| gpio_config_t io_conf = {}; | |
| io_conf.intr_type = GPIO_INTR_DISABLE; | |
| io_conf.mode = GPIO_MODE_INPUT; | |
| io_conf.pin_bit_mask = (1ULL << board::encoder::kA) | | |
| (1ULL << board::encoder::kB) | | |
| (1ULL << board::encoder::kSwitch) | | |
| (1ULL << board::controls::kBypassButton); | |
| io_conf.pull_down_en = GPIO_PULLDOWN_DISABLE; | |
| io_conf.pull_up_en = GPIO_PULLUP_ENABLE; | |
| esp_err_t err = gpio_config(&io_conf); | |
| if (err != ESP_OK) { | |
| ESP_LOGE(kTag, "gpio_config falhou: %s", esp_err_to_name(err)); | |
| return false; | |
| } | |
| // Inicializa o estado do encoder | |
| lastEncoderA_ = gpio_get_level(static_cast<gpio_num_t>(board::encoder::kA)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@targets/embedded_esp32s3/main/ui_tft.cpp` around lines 30 - 43, The
gpio_config call result is unchecked and the comment has a typo; update the GPIO
configuration block around gpio_config and io_conf to capture the esp_err_t
return (from gpio_config(&io_conf)), check for non-ESP_OK, and log or handle the
failure via the existing logging mechanism (include context like "gpio_config
failed for encoder/buttons" and the error code), and correct the comment text
"Incializa o estado do encoder" to "Inicializa o estado do encoder"; also ensure
the subsequent use of lastEncoderA_ (set via
gpio_get_level(static_cast<gpio_num_t>(board::encoder::kA))) remains after the
error check so it only runs when gpio_config succeeds.
| void UiTft::processInputs() { | ||
| uint32_t now = xTaskGetTickCount() * portTICK_PERIOD_MS; | ||
|
|
||
| // Encoder - Leitura baseada em polling de quadratura | ||
| int a = gpio_get_level(static_cast<gpio_num_t>(board::encoder::kA)); | ||
| int b = gpio_get_level(static_cast<gpio_num_t>(board::encoder::kB)); | ||
|
|
||
| if (a != lastEncoderA_) { | ||
| if (a == 0) { // Na borda de descida de A | ||
| if (b == 1) { | ||
| encoderPos_++; // CW | ||
| } else { | ||
| encoderPos_--; // CCW | ||
| } | ||
| } | ||
| lastEncoderA_ = a; | ||
| } | ||
|
|
||
| // Encoder Switch (Debounced) | ||
| bool switchState = gpio_get_level(static_cast<gpio_num_t>(board::encoder::kSwitch)); | ||
| if (switchState != lastSwitchState_) { | ||
| if (now - switchDebounceTime_ > 50) { // 50ms debounce | ||
| if (switchState == 0) { // Pressionado (pull-up) | ||
| switchPressed_ = true; | ||
| } | ||
| lastSwitchState_ = switchState; | ||
| switchDebounceTime_ = now; | ||
| } | ||
| } | ||
|
|
||
| // Bypass Button (Debounced) | ||
| bool bypassState = gpio_get_level(static_cast<gpio_num_t>(board::controls::kBypassButton)); | ||
| if (bypassState != lastBypassState_) { | ||
| if (now - bypassDebounceTime_ > 50) { // 50ms debounce | ||
| if (bypassState == 0) { // Pressionado (pull-up) | ||
| bypassPressed_ = true; | ||
| } | ||
| lastBypassState_ = bypassState; | ||
| bypassDebounceTime_ = now; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ESP32-S3 ESP-IDF rotary encoder PCNT pulse counter quadrature example
💡 Result:
The official ESP-IDF example for using the PCNT (Pulse Counter) peripheral with a rotary encoder quadrature signals on ESP32-S3 is located at examples/peripherals/pcnt/rotary_encoder. This example decodes quadrature signals from a common EC11 rotary encoder using PCNT units and channels. Key steps from the example code (rotary_encoder_example_main.c): 1. Create PCNT unit with high/low limits (e.g., 100 / -100): pcnt_unit_config_t unit_config = { .high_limit = EXAMPLE_PCNT_HIGH_LIMIT, .low_limit = EXAMPLE_PCNT_LOW_LIMIT, }; ESP_ERROR_CHECK(pcnt_new_unit(&unit_config, &pcnt_unit)); 2. Set glitch filter (e.g., 1000 ns): pcnt_glitch_filter_config_t filter_config = { .max_glitch_ns = 1000, }; ESP_ERROR_CHECK(pcnt_unit_set_glitch_filter(pcnt_unit, &filter_config)); 3. Create two channels for quadrature A/B signals (e.g., GPIO 0 and 2): pcnt_chan_config_t chan_a_config = { .edge_gpio_num = EXAMPLE_EC11_GPIO_A, // GPIO A .level_gpio_num = EXAMPLE_EC11_GPIO_B, // GPIO B as control }; ESP_ERROR_CHECK(pcnt_new_channel(pcnt_unit, &chan_a_config, &pcnt_chan_a)); pcnt_chan_config_t chan_b_config = { .edge_gpio_num = EXAMPLE_EC11_GPIO_B, .level_gpio_num = EXAMPLE_EC11_GPIO_A, }; ESP_ERROR_CHECK(pcnt_new_channel(pcnt_unit, &chan_b_config, &pcnt_chan_b)); 4. Set edge and level actions for 4x quadrature decoding (increase/decrease/hold based on edges and levels): ESP_ERROR_CHECK(pcnt_channel_set_edge_action(pcnt_chan_a, PCNT_CHANNEL_EDGE_ACTION_HOLD, PCNT_CHANNEL_EDGE_ACTION_INCREASE)); ESP_ERROR_CHECK(pcnt_channel_set_edge_action(pcnt_chan_b, PCNT_CHANNEL_EDGE_ACTION_HOLD, PCNT_CHANNEL_EDGE_ACTION_DECREASE)); ESP_ERROR_CHECK(pcnt_channel_set_level_action(pcnt_chan_a, PCNT_CHANNEL_LEVEL_ACTION_KEEP, PCNT_CHANNEL_LEVEL_ACTION_INCREASE)); ESP_ERROR_CHECK(pcnt_channel_set_level_action(pcnt_chan_b, PCNT_CHANNEL_LEVEL_ACTION_KEEP, PCNT_CHANNEL_LEVEL_ACTION_DECREASE)); 5. Add watch points (e.g., -100, -50, 0, 50, 100) and register event callbacks. 6. Enable, clear, and start the unit: ESP_ERROR_CHECK(pcnt_unit_enable(pcnt_unit)); ESP_ERROR_CHECK(pcnt_unit_clear_count(pcnt_unit)); ESP_ERROR_CHECK(pcnt_unit_start(pcnt_unit)); 7. Read count periodically with pcnt_unit_get_count. This supports ESP32-S3 as confirmed in ESP-IDF docs (v5.0+), where PCNT is used as quadrature decoder. Clone ESP-IDF repo, build/flash the example, adjust GPIOs for your wiring (pull-ups recommended). Full code: https://github.com/espressif/esp-idf/blob/master/examples/peripherals/pcnt/rotary_encoder/main/rotary_encoder_example_main.c To build: idf.py set-target esp32s3 menuconfig flash monitor
Citations:
- 1: https://docs.espressif.com/projects/esp-idf/en/stable/esp32s3/api-reference/peripherals/pcnt.html
- 2: https://docs.espressif.com/projects/esp-idf/en/v5.3.5/esp32s3/api-reference/peripherals/pcnt.html
- 3: https://docs.espressif.com/projects/esp-idf/en/v5.0/esp32s3/api-reference/peripherals/pcnt.html
- 4: https://github.com/espressif/esp-idf/blob/master/examples/peripherals/pcnt/rotary_encoder/main/rotary_encoder_example_main.c
- 5: https://github.com/espressif/esp-idf/tree/master/examples/peripherals/pcnt/rotary_encoder
- 6: https://github.com/espressif/esp-idf/tree/v5.4.1/examples/peripherals/pcnt/rotary_encoder
- 7: https://github.com/espressif/esp-idf/tree/v5.5.3/examples/peripherals/pcnt/rotary_encoder
🏁 Script executed:
# Find the UiTft class and understand the refresh period
fd -e hpp -e cpp -e h | xargs rg -l "UiTft" | head -5Repository: ovelhaaa/orbit-echo
Length of output: 185
🏁 Script executed:
# Search for refreshPeriodMs definition
rg "refreshPeriodMs\s*=" --type cpp --type hpp -A 2Repository: ovelhaaa/orbit-echo
Length of output: 90
🏁 Script executed:
# Find taskLoop or processInputs calls
rg "processInputs|taskLoop" --type cpp --type hpp -B 2 -A 2Repository: ovelhaaa/orbit-echo
Length of output: 90
🏁 Script executed:
# Check the full processInputs context and task definition
rg "void.*processInputs|class UiTft" --type cpp --type hpp -A 1Repository: ovelhaaa/orbit-echo
Length of output: 90
🏁 Script executed:
# Search for refreshPeriodMs in the found files
rg "refreshPeriodMs" targets/embedded_esp32s3/main/ -A 2 -B 2Repository: ovelhaaa/orbit-echo
Length of output: 1135
🏁 Script executed:
# Search for processInputs calls and context
rg "processInputs|taskLoop" targets/embedded_esp32s3/main/ -A 3 -B 1Repository: ovelhaaa/orbit-echo
Length of output: 1691
🏁 Script executed:
# Check UiTft header for task-related members
cat targets/embedded_esp32s3/main/ui_tft.h | head -100Repository: ovelhaaa/orbit-echo
Length of output: 1609
🏁 Script executed:
# Check app_main to see how the UI task is scheduled
cat targets/embedded_esp32s3/main/app_main.cpp | head -150Repository: ovelhaaa/orbit-echo
Length of output: 5071
Polling the encoder at the UI refresh rate (33 ms) will miss pulses.
processInputs() only samples encoder A/B once per taskLoop iteration (~33 ms → ~30 Hz). Mechanical rotary encoders produce edges much faster than that, so most transitions—and sometimes direction reversals—are lost, causing erratic/sluggish knob behavior. Single-edge (A falling) detection further halves resolution and makes missed-edge direction errors more likely.
Recommended options:
- Use the ESP32-S3 PCNT (pulse counter) peripheral with quadrature mode — it handles debouncing and decoding in hardware and is the idiomatic approach in ESP-IDF (see
examples/peripherals/pcnt/rotary_encoder). - Use GPIO interrupts on A (and B) and queue deltas from the ISR to the UI task.
- At minimum, poll at a much higher rate (a dedicated 1–2 ms timer/task) and decode full-quadrature state transitions rather than a single edge.
Leaving the current loop in place will make the on-device UX noticeably worse than it appears in unit tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@targets/embedded_esp32s3/main/ui_tft.cpp` around lines 95 - 136,
processInputs() currently polls encoder A/B once per UI loop (~33ms) and only on
A falling edge, causing missed pulses and wrong direction; fix by replacing the
polling logic in UiTft::processInputs with one of: (1) use ESP32-S3 PCNT in
quadrature mode to maintain encoderPos_ in hardware, (2) attach GPIO interrupts
(gpio_isr_handler_add) for A and B and push deltas to a FreeRTOS queue consumed
by the UI task to update encoderPos_, or (3) if hardware/ISR changes are not
possible immediately, move decoding to a high-rate dedicated task/timer (1–2ms)
and implement full-quadrature state decoding (track last state of A/B and decode
all transitions) instead of only checking lastEncoderA_; also keep
debounce/press handling for lastSwitchState_/switchDebounceTime_ and
lastBypassState_/bypassDebounceTime_ but perform them in the ISR-safe queue/fast
task model if using interrupts.
| // Sincroniza parâmetros iniciais | ||
| if (config_.paramBridge) { | ||
| // Se o paramBridge tivesse método getter, a gente puxava. | ||
| // Vamos inicializar com alguns valores padrão conhecidos, | ||
| // alinhados com o estado default da struct AudioParams | ||
| currentParams_.mix = 0.35f; | ||
| currentParams_.feedback = 0.35f; | ||
| currentParams_.offsetSamples = 1200.0f; | ||
| currentParams_.toneHz = 8000.0f; | ||
| } |
There was a problem hiding this comment.
Initial currentParams_ is out of sync with what app_main just published.
app_main() publishes initialParams with readMode = Accidental and dcBlockEnabled = true (plus whatever AudioParams{} defaults the struct gives the rest). Here, the UI only overwrites four fields on a default-constructed currentParams_; the other fields (orbit, stereoSpread, inputGain, outputGain, smearAmount, diffuserStages, dcBlockEnabled, readMode) hold whatever AudioParams{} happens to produce. The first time the user touches a knob or the bypass button, config_.paramBridge->publish(currentParams_) will overwrite all atomics — silently reverting readMode/dcBlockEnabled and any other field whose default differs across the two sides.
This is the root cause; the header comment on currentParams_ points back here.
Two reasonable fixes:
- Add a
consume/ getter onParameterBridgeand seedcurrentParams_from it at task start (preferred — single source of truth). - Centralize the initial
AudioParamsin one place (e.g., akDefaultAudioParamsconstant) that bothapp_mainandUiTft::taskLoopuse to seed their local copies.
Until this is addressed, any UI interaction effectively resets audio-engine state that was intentionally configured at boot.
🛠️ Sketch of option 1
- // Sincroniza parâmetros iniciais
- if (config_.paramBridge) {
- // Se o paramBridge tivesse método getter, a gente puxava.
- // Vamos inicializar com alguns valores padrão conhecidos,
- // alinhados com o estado default da struct AudioParams
- currentParams_.mix = 0.35f;
- currentParams_.feedback = 0.35f;
- currentParams_.offsetSamples = 1200.0f;
- currentParams_.toneHz = 8000.0f;
- }
+ // Seed local mirror from the bridge so the first publish doesn't stomp
+ // fields the UI never touches (e.g., readMode, dcBlockEnabled).
+ if (config_.paramBridge) {
+ (void)config_.paramBridge->snapshot(currentParams_); // new helper on ParameterBridge
+ }Add a snapshot() (or peek()) method on ParameterBridge that loads every atomic into an AudioParams without touching lastConsumedVersion_.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@targets/embedded_esp32s3/main/ui_tft.cpp` around lines 208 - 217,
currentParams_ in UiTft::taskLoop is only partially initialized which causes
publish(config_.paramBridge->publish(currentParams_)) to overwrite app_main's
initialParams; fix by either (preferred) adding a snapshot/peek method on
ParameterBridge that reads all atomics into an AudioParams without advancing
lastConsumedVersion_ and call it from UiTft::taskLoop to seed currentParams_
(use symbols: ParameterBridge::snapshot()/peek(), UiTft::taskLoop,
currentParams_, config_.paramBridge) or (alternative) centralize the boot
defaults into a single kDefaultAudioParams constant used by both app_main
(initialParams) and UiTft to initialize currentParams_ so both sides share the
same full set of fields (use symbols: kDefaultAudioParams, app_main,
UiTft::taskLoop, currentParams_).
| float preBypassMix_ = 0.35f; | ||
|
|
||
| // UI Parameters | ||
| enum class Page { | ||
| Mix = 0, | ||
| Feedback, | ||
| Time, | ||
| Tone, | ||
| Count | ||
| }; | ||
| Page currentPage_ = Page::Mix; | ||
|
|
||
| AudioParams currentParams_; |
There was a problem hiding this comment.
currentParams_ defaulted in-class may drift from ParameterBridge/AudioParams defaults.
currentParams_ is default-constructed here and then only partially re-initialized in UiTft::taskLoop() (mix/feedback/offsetSamples/toneHz). All other fields (orbit, stereoSpread, inputGain, outputGain, smearAmount, diffuserStages, dcBlockEnabled, readMode) remain whatever AudioParams{} yields — which is not guaranteed to match the ParameterBridge constructor defaults or the initialParams published from app_main.cpp (where readMode = Accidental and dcBlockEnabled = true are set explicitly). The first UI publish will stomp those fields on the audio side.
See the corresponding comment on ui_tft.cpp (Line 209-217) for the fix — typically either expose a getter on ParameterBridge and seed currentParams_ from it, or centralize the default AudioParams so both sides stay in sync.
Same concern for preBypassMix_ = 0.35f; — it duplicates the initial mix default and will go stale if the shared default changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@targets/embedded_esp32s3/main/ui_tft.h` around lines 57 - 69, currentParams_
and preBypassMix_ should not be hard-defaulted in-class; instead initialize them
from the authoritative source so UI and audio defaults stay in sync: remove the
in-class defaults and in UiTft's constructor (or initialization path) copy the
live defaults from ParameterBridge (e.g. call
ParameterBridge::getCurrentParams() or a new getter that returns the initial
AudioParams) into currentParams_, and set preBypassMix_ from that same
AudioParams::mix (or a centralized default); also ensure UiTft::taskLoop() only
updates the fields it intends to change and does not overwrite the other
AudioParams fields. Use the symbols currentParams_, UiTft::taskLoop(),
ParameterBridge, AudioParams, and preBypassMix_ to locate where to make the
change.
1 participant
Implemented the ESP32-S3 hardware UI to run on a dedicated task, completely decoupled from the audio DSP loop, using
ParameterBridge. Includes inputs processing for an encoder and a bypass button. Adds SVG mockups for visual representations.PR created automatically by Jules for task 1164950496833111320 started by @ovelhaaa
Summary by CodeRabbit