Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions docs/ui/screen_bypass.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
15 changes: 15 additions & 0 deletions docs/ui/screen_feedback.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
15 changes: 15 additions & 0 deletions docs/ui/screen_mix.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
15 changes: 5 additions & 10 deletions targets/embedded_esp32s3/main/app_main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -130,16 +130,10 @@ void audioCallback(void* userData, const int32_t* inInterleaved, int32_t* outInt
}

void uiTick(void* userData) {
auto* app = static_cast<AppContext*>(userData);
static uint32_t tick = 0;
++tick;

AudioParams p;
p.mix = 0.30f + 0.15f * ((tick / 120) % 2);
p.feedback = 0.40f;
p.toneHz = 6500.0f;
p.smearAmount = 0.20f;
app->params.publish(p);
// Opcional: A UI task já trata seus inputs e manda os parâmetros pro bridge.
// O uiTick agora poderia ser usado apenas para side-effects ou heartbeats,
// mas o loop principal da UI task gerencia o estado através de UiTft::updateState().
// Removendo a lógica dummy que alterava os parâmetros.
}

} // namespace
Expand Down Expand Up @@ -190,6 +184,7 @@ extern "C" void app_main(void) {
uiCfg.refreshPeriodMs = board::ui::kRefreshPeriodMs;
uiCfg.core = board::ui::kTaskCore;
uiCfg.priority = board::ui::kTaskPriority;
uiCfg.paramBridge = &app.params;

if (!app.ui.start(uiCfg, uiTick, &app)) {
ESP_LOGW(kTag, "UI não iniciou; áudio segue ativo");
Expand Down
145 changes: 145 additions & 0 deletions targets/embedded_esp32s3/main/ui_tft.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
#include "esp_log.h"
#include "freertos/FreeRTOS.h"
#include "freertos/task.h"
#include "driver/gpio.h"
#include "board_config.h"

#include <algorithm>
#include <cmath>

namespace orbit::embedded {
namespace {
Expand All @@ -22,6 +27,21 @@ bool UiTft::start(const Config& config, UiTickCallback callback, void* userData)
callback_ = callback;
userData_ = userData;

// 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));
Comment on lines +30 to +43

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 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) returns esp_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.

Suggested change
// 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.


if (!stoppedSignal_) {
stoppedSignal_ = xSemaphoreCreateBinary();
if (!stoppedSignal_) {
Expand Down Expand Up @@ -72,14 +92,139 @@ void UiTft::taskEntry(void* ctx) {
static_cast<UiTft*>(ctx)->taskLoop();
}

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;
}
}
}
Comment on lines +95 to +136

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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:


🏁 Script executed:

# Find the UiTft class and understand the refresh period
fd -e hpp -e cpp -e h | xargs rg -l "UiTft" | head -5

Repository: ovelhaaa/orbit-echo

Length of output: 185


🏁 Script executed:

# Search for refreshPeriodMs definition
rg "refreshPeriodMs\s*=" --type cpp --type hpp -A 2

Repository: ovelhaaa/orbit-echo

Length of output: 90


🏁 Script executed:

# Find taskLoop or processInputs calls
rg "processInputs|taskLoop" --type cpp --type hpp -B 2 -A 2

Repository: 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 1

Repository: 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 2

Repository: 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 1

Repository: 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 -100

Repository: 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 -150

Repository: 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.


void UiTft::updateState() {
bool paramsUpdated = false;

// Trata o botão Bypass
if (bypassPressed_) {
isBypassed_ = !isBypassed_;
bypassPressed_ = false;

// Em caso de bypass, podemos mudar o readMode ou zerar o mix.
// Aqui assumimos zerar o mix como forma de bypass.
if (isBypassed_) {
preBypassMix_ = currentParams_.mix;
currentParams_.mix = 0.0f;
} else {
currentParams_.mix = preBypassMix_;
}
paramsUpdated = true;
}

// Trata o botão do Encoder (Muda a página)
if (switchPressed_) {
int nextPage = static_cast<int>(currentPage_) + 1;
if (nextPage >= static_cast<int>(Page::Count)) {
nextPage = 0;
}
currentPage_ = static_cast<Page>(nextPage);
switchPressed_ = false;
}

// Trata a rotação do Encoder (Muda os valores da página atual)
if (encoderPos_ != 0) {
if (!isBypassed_) { // Ignora rotações no bypass, ou desabilita bypass?
switch (currentPage_) {
case Page::Mix:
currentParams_.mix = std::clamp(currentParams_.mix + (encoderPos_ * 0.05f), 0.0f, 1.0f);
break;
case Page::Feedback:
currentParams_.feedback = std::clamp(currentParams_.feedback + (encoderPos_ * 0.05f), 0.0f, 1.0f);
break;
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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

medium

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.

@jules

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

break;
case Page::Tone:
currentParams_.toneHz = std::clamp(currentParams_.toneHz + (encoderPos_ * 500.0f), 200.0f, 20000.0f);
break;
default:
break;
}
paramsUpdated = true;
}
encoderPos_ = 0;
}

// Publica se houve mudança real e o ParameterBridge estiver configurado
if (paramsUpdated && config_.paramBridge) {
config_.paramBridge->publish(currentParams_);
}
}

void UiTft::drawDisplay() {
// Stub: Simular o desenho do display
// Em uma implementação real usaria SPI para enviar os dados da tela
}


void UiTft::taskLoop() {
const TickType_t periodTicks = pdMS_TO_TICKS(config_.refreshPeriodMs);
TickType_t lastWake = xTaskGetTickCount();

// 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;
}
Comment on lines +208 to +217

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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:

  1. Add a consume / getter on ParameterBridge and seed currentParams_ from it at task start (preferred — single source of truth).
  2. Centralize the initial AudioParams in one place (e.g., a kDefaultAudioParams constant) that both app_main and UiTft::taskLoop use 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_).


while (running_) {
processInputs();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@jules address

high

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.

@jules

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

updateState();
drawDisplay();

if (callback_) {
callback_(userData_);
}

vTaskDelayUntil(&lastWake, periodTicks);
}

Expand Down
32 changes: 32 additions & 0 deletions targets/embedded_esp32s3/main/ui_tft.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "freertos/FreeRTOS.h"
#include "freertos/semphr.h"
#include "freertos/task.h"
#include "parameter_bridge.h"

namespace orbit::embedded {

Expand All @@ -15,6 +16,7 @@ class UiTft {
uint32_t stackWords = 4096;
uint32_t priority = 3;
int core = 1;
ParameterBridge* paramBridge = nullptr;
};

using UiTickCallback = void (*)(void* userData);
Expand All @@ -29,12 +31,42 @@ class UiTft {
static void taskEntry(void* ctx);
void taskLoop();

void processInputs();
void updateState();
void drawDisplay();

Config config_{};
UiTickCallback callback_ = nullptr;
void* userData_ = nullptr;
TaskHandle_t taskHandle_ = nullptr;
SemaphoreHandle_t stoppedSignal_ = nullptr;
bool running_ = false;

// Encoder State
int lastEncoderA_ = 1;
int encoderPos_ = 0;
bool switchPressed_ = false;
bool lastSwitchState_ = true;
uint32_t switchDebounceTime_ = 0;

// Bypass State
bool bypassPressed_ = false;
bool isBypassed_ = false;
bool lastBypassState_ = true;
uint32_t bypassDebounceTime_ = 0;
float preBypassMix_ = 0.35f;

// UI Parameters
enum class Page {
Mix = 0,
Feedback,
Time,
Tone,
Count
};
Page currentPage_ = Page::Mix;

AudioParams currentParams_;
Comment on lines +57 to +69

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

};

} // namespace orbit::embedded