LFO+ App #392
LFO+ App #392
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a new LFO+ application (appId 22) with full implementation, registration in the app registry, and manual documentation entry; includes parameters, persistent storage, an async run loop wiring hardware I/O, LFO computation, MIDI CC output, and LED feedback. Changes
Sequence DiagramsequenceDiagram
participant HW as Hardware (Faders/Buttons/Clock)
participant App as LFO+ App (run)
participant Params as ParamStore / Storage
participant Compute as LFO Computation
participant MIDI as MIDI Output
participant LED as LED Control
loop Event loop
HW->>App: Input events (fader/button/clock tick)
App->>Params: Load current params/state
App->>Compute: Calculate LFO value (waveform, speed, phase, range)
Compute-->>App: Computed value
alt Value changed or gated
App->>MIDI: Emit MIDI CC for destination
MIDI-->>App: Ack
end
App->>LED: Update LEDs (waveform, latch/layer state)
LED-->>App: Updated
alt Scene save/load triggered
App->>Params: Persist or load Storage
Params-->>App: Done
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 5
🤖 Fix all issues with AI agents
In @configurator/src/components/ManualTab.tsx:
- Line 824: The storage array currently contains a single string with multiple
comma-separated values; update the storage array (in ManualTab.tsx where the
storage property is defined) so each storage item is its own string element
instead of one combined string — e.g., replace the single element "CV
attenuation, CV mute, CV destination, Clocked" with separate elements "CV
attenuation", "CV mute", "CV destination", "Clocked" while leaving the existing
"Attenuation", "Speed", and "Waveform" entries as they are.
In @faderpunk/src/apps/lfo_plus.rs:
- Around line 461-462: The LED for the waveform is being set on channel 0 after
scene load; update the leds.set call that uses get_color_for(wave_saved) so it
targets channel 1 (the LFO output) instead of 0 to match the initialization
behavior (change the leds.set(0, Led::Button, ...) to leds.set(1, Led::Button,
...)). Ensure you modify the call that runs after scene loading where wave_saved
is used so the waveform LED consistently lights the LFO output channel.
- Around line 407-412: The local variable `dest` is assigned from the call to
`storage.modify_and_save` but never used; remove the unused binding by either
discarding the return value (e.g., call `storage.modify_and_save(...)` without
`let dest =`) or explicitly ignore it with `let _ =
storage.modify_and_save(...)` so the `s.dest` increment still happens; update
the code around `if chan == 0 { ... }` that calls `storage.modify_and_save` and
eliminate the unused `dest` variable.
- Around line 482-490: The Saw waveform is currently mapped to Color::Cyan in
get_color_for (e.g., Waveform::Saw => Color::Cyan) but the docs call it "ramp
down (blue)"; reconcile by choosing one approach: either change the mapping in
get_color_for (and the equivalent functions in lfo.rs and panner.rs) to use
Color::Blue for Waveform::Saw, or update the ManualTab.tsx documentation text to
say "cyan" instead of "blue"; locate the get_color_for functions and the
Waveform::Saw matches across lfo_plus.rs, lfo.rs, and panner.rs and make the
consistent change across all three places so code and docs match.
🧹 Nitpick comments (1)
faderpunk/src/apps/lfo_plus.rs (1)
81-91: Consider validating slice length infrom_values.The
from_valuesimplementation directly accesses indices 0-5 without bounds checking. If the slice is shorter than expected, this would panic. Consider adding a length check, though this may be consistent with existing app patterns.Optional defensive check
impl AppParams for Params { fn from_values(values: &[Value]) -> Option<Self> { + if values.len() < 6 { + return None; + } Some(Self { speed_mult: usize::from_value(values[0]), range: Range::from_value(values[1]), midi_out: MidiOut::from_value(values[2]), midi_channel: MidiChannel::from_value(values[3]), midi_cc: MidiCc::from_value(values[4]), color_in: Color::from_value(values[5]), }) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
configurator/src/components/ManualTab.tsxfaderpunk/src/apps/lfo_plus.rsfaderpunk/src/apps/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Firmware (Check)
🔇 Additional comments (6)
faderpunk/src/apps/mod.rs (1)
21-21: Verify the app ID gap is intentional.The new app is registered with ID 22, but IDs 20 and 21 are not present in the registry (the previous entry is 19 => panner). If these IDs are reserved for future apps, this is fine. Otherwise, consider using ID 20 to maintain sequential ordering.
configurator/src/components/ManualTab.tsx (1)
817-862: Documentation entry looks good overall.The LFO+ app entry correctly documents the two-channel configuration with Input and Output channels. The text description comprehensively covers the app's functionality, waveform selection, CV destination modes, and MIDI output capabilities.
faderpunk/src/apps/lfo_plus.rs (4)
23-57: Configuration looks correct.The CONFIG definition with 6 parameters aligns with the PARAMS constant, and the parameter definitions follow the established pattern from other apps.
105-130: Storage struct is well-defined.The Storage struct with serialization support and sensible defaults follows the established pattern. The inline comment on
destdocumenting the magic numbers (0 => speed, 1 => phase, 2 => amp) is helpful.
132-152: Wrapper function follows the standard app pattern.The wrapper correctly initializes stores, loads persisted state, and runs the main loop with proper exit signal handling.
214-332: Main LFO loop implementation is sound.The 1ms polling loop handles CV input processing, LFO computation with destination-based modulation (speed/phase/amplitude), and proper LED feedback for different latch layers. The MIDI CC quantization (line 277:
val / 32) appropriately reduces message frequency.
| color: "Yellow", | ||
| icon: "sine", | ||
| params: ["Speed division", "Range", "Send MIDI", "MIDI Channel", "MIDI CC"], | ||
| storage: ["CV attenuation, CV mute, CV destination, Clocked", "Attenuation", "Speed", "Waveform"], |
There was a problem hiding this comment.
Fix the storage array formatting.
The first element of the storage array contains multiple comma-separated values in a single string, which is inconsistent with other app entries. Each storage item should be a separate array element.
Proposed fix
- storage: ["CV attenuation, CV mute, CV destination, Clocked", "Attenuation", "Speed", "Waveform"],
+ storage: ["CV attenuation", "CV mute", "CV destination", "Clocked", "Attenuation", "Speed", "Waveform"],📝 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.
| storage: ["CV attenuation, CV mute, CV destination, Clocked", "Attenuation", "Speed", "Waveform"], | |
| storage: ["CV attenuation", "CV mute", "CV destination", "Clocked", "Attenuation", "Speed", "Waveform"], |
🤖 Prompt for AI Agents
In @configurator/src/components/ManualTab.tsx at line 824, The storage array
currently contains a single string with multiple comma-separated values; update
the storage array (in ManualTab.tsx where the storage property is defined) so
each storage item is its own string element instead of one combined string —
e.g., replace the single element "CV attenuation, CV mute, CV destination,
Clocked" with separate elements "CV attenuation", "CV mute", "CV destination",
"Clocked" while leaving the existing "Attenuation", "Speed", and "Waveform"
entries as they are.
| let div = resolution | ||
| [((storage.query(|s| s.layer_speed) + offset - 2047) as usize / 500).clamp(0, 8)]; | ||
| glob_quant_speed.set(4095. / ((glob_count.get().max(1) as f32 * div as f32) / 24.)); |
There was a problem hiding this comment.
Potential integer underflow in division calculation.
When layer_speed + offset < 2047, the subtraction will underflow for unsigned arithmetic. Since layer_speed and offset are both u16, if their sum is less than 2047, this causes an underflow before the as usize cast.
Consider using saturating arithmetic or casting to signed integers first:
Proposed fix using saturating subtraction
let div = resolution
- [((storage.query(|s| s.layer_speed) + offset - 2047) as usize / 500).clamp(0, 8)];
+ [((storage.query(|s| s.layer_speed) as i32 + offset as i32 - 2047).max(0) as usize / 500).clamp(0, 8)];📝 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.
| let div = resolution | |
| [((storage.query(|s| s.layer_speed) + offset - 2047) as usize / 500).clamp(0, 8)]; | |
| glob_quant_speed.set(4095. / ((glob_count.get().max(1) as f32 * div as f32) / 24.)); | |
| let div = resolution | |
| [((storage.query(|s| s.layer_speed) as i32 + offset as i32 - 2047).max(0) as usize / 500).clamp(0, 8)]; | |
| glob_quant_speed.set(4095. / ((glob_count.get().max(1) as f32 * div as f32) / 24.)); |
| let color = get_color_for(wave_saved); | ||
| leds.set(0, Led::Button, color, Brightness::Lower); |
There was a problem hiding this comment.
LED set on wrong channel after scene load.
After loading a scene, the waveform color LED is set on channel 0 (CV input), but it should be set on channel 1 (LFO output) to match the initialization at line 189.
Proposed fix
let color = get_color_for(wave_saved);
- leds.set(0, Led::Button, color, Brightness::Lower);
+ leds.set(1, Led::Button, color, Brightness::Lower);📝 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.
| let color = get_color_for(wave_saved); | |
| leds.set(0, Led::Button, color, Brightness::Lower); | |
| let color = get_color_for(wave_saved); | |
| leds.set(1, Led::Button, color, Brightness::Lower); |
🤖 Prompt for AI Agents
In @faderpunk/src/apps/lfo_plus.rs around lines 461 - 462, The LED for the
waveform is being set on channel 0 after scene load; update the leds.set call
that uses get_color_for(wave_saved) so it targets channel 1 (the LFO output)
instead of 0 to match the initialization behavior (change the leds.set(0,
Led::Button, ...) to leds.set(1, Led::Button, ...)). Ensure you modify the call
that runs after scene loading where wave_saved is used so the waveform LED
consistently lights the LFO output channel.
| fn get_color_for(wave: Waveform) -> Color { | ||
| match wave { | ||
| Waveform::Sine => Color::Yellow, | ||
| Waveform::Triangle => Color::Pink, | ||
| Waveform::Saw => Color::Cyan, | ||
| Waveform::SawInv => Color::Red, | ||
| Waveform::Square => Color::White, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, confirm the current state of the get_color_for function in lfo_plus.rs
cat -n faderpunk/src/apps/lfo_plus.rs | sed -n '482,490p'Repository: ATOVproject/faderpunk
Length of output: 400
🏁 Script executed:
# Search for documentation references to color mappings, especially for Saw/ramp down
rg -n "ramp down|Saw.*Color|Color::Cyan|Color::Blue" --type rust --type javascript --type typescript -iRepository: ATOVproject/faderpunk
Length of output: 99
🏁 Script executed:
# Search for ManualTab.tsx to check the documentation mentioned
fd -name "ManualTab.tsx" -o -name "*manual*" | head -20Repository: ATOVproject/faderpunk
Length of output: 295
🏁 Script executed:
# Search for ManualTab file without using fd -n flag
find . -name "*ManualTab*" -o -name "*manual*" | grep -i manual | head -20Repository: ATOVproject/faderpunk
Length of output: 145
🏁 Script executed:
# Search for Color enum definitions and see what colors are available
rg -n "enum Color|Color::" --type rust -A 2 | head -80Repository: ATOVproject/faderpunk
Length of output: 4070
🏁 Script executed:
# Search for documentation/comments mentioning color mappings
rg -n "ramp down|ramp up|sine.*yellow|triangle.*pink" -iRepository: ATOVproject/faderpunk
Length of output: 3398
🏁 Script executed:
# Get the actual RGB/color values for CYAN and BLUE
rg -n "CYAN\s*=|BLUE\s*=" libfp/src/lib.rs -A 0Repository: ATOVproject/faderpunk
Length of output: 47
🏁 Script executed:
# Check how other LFO implementations map Saw waveform
rg -n "Waveform::Saw.*Color" faderpunk/src/apps/ --type rustRepository: ATOVproject/faderpunk
Length of output: 494
🏁 Script executed:
# Also check SawInv mapping in all places
rg -n "Waveform::SawInv.*Color" faderpunk/src/apps/ --type rustRepository: ATOVproject/faderpunk
Length of output: 282
🏁 Script executed:
# Get the color constant definitions
cat -n libfp/src/lib.rs | sed -n '534,600p' | grep -E "CYAN|BLUE"Repository: ATOVproject/faderpunk
Length of output: 242
🏁 Script executed:
# Search for where these constants are defined
rg -n "const CYAN|const BLUE|CYAN\s*=" libfp/src/ --type rustRepository: ATOVproject/faderpunk
Length of output: 192
🏁 Script executed:
# Get the full definitions of CYAN and BLUE constants with their RGB values
cat -n libfp/src/colors.rs | sed -n '35,45p'Repository: ATOVproject/faderpunk
Length of output: 331
Reconcile Saw waveform color mapping with documentation.
The code maps Waveform::Saw to Color::Cyan (RGB: 0, 255, 186), but the documentation in ManualTab.tsx consistently describes "ramp down (blue)". Color::Cyan and Color::Blue (RGB: 6, 0, 255) are distinct colors. This discrepancy exists across all three LFO implementations (lfo.rs, lfo_plus.rs, panner.rs). Either update the code to use Color::Blue or update the documentation to reference "cyan".
🤖 Prompt for AI Agents
In @faderpunk/src/apps/lfo_plus.rs around lines 482 - 490, The Saw waveform is
currently mapped to Color::Cyan in get_color_for (e.g., Waveform::Saw =>
Color::Cyan) but the docs call it "ramp down (blue)"; reconcile by choosing one
approach: either change the mapping in get_color_for (and the equivalent
functions in lfo.rs and panner.rs) to use Color::Blue for Waveform::Saw, or
update the ManualTab.tsx documentation text to say "cyan" instead of "blue";
locate the get_color_for functions and the Waveform::Saw matches across
lfo_plus.rs, lfo.rs, and panner.rs and make the consistent change across all
three places so code and docs match.
ArthurGibert
force-pushed
the
feat/app-lfo-plus
branch
from
1f9ea22 to
67d0d3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @faderpunk/src/apps/lfo_plus.rs:
- Around line 203-212: The closure time_calc performs arithmetic on two u16
values that can overflow when adding storage.query(|s| s.layer_speed) and
offset; change the computation to use a wider integer or saturating/checked
arithmetic before subtracting 2047 and indexing resolution: e.g. cast both
layer_speed and offset to u32 (or use .saturating_add) then subtract
2047_i32/u32 safely, clamp the result to the valid range, convert to usize and
divide by 500 to compute the index; update the expression that computes div (the
resolution[...] index) and any subsequent casts so all intermediate arithmetic
happens in the larger type to avoid u16 overflow.
- Around line 424-431: The LED flash uses the previously captured `color`
instead of the current waveform color; after toggling `s.clocked` in
`storage.modify_and_save` (or immediately after that call), read the current
waveform/color from the storage state (e.g., `s.waveform` or a helper like
`waveform_color(&s.waveform)`) and pass that computed color into
`leds.set_mode(1, Led::Button, LedMode::Flash(current_color, Some(4)))` so the
flash matches the updated waveform.
- Around line 453-463: The match arm uses the misspelled variant
SceneEvent::LoadSscene and writes the waveform color to LED index 0; change the
pattern to SceneEvent::LoadScene (matching the enum variant) and update the
leds.set call to target LED index 1 (i.e., replace leds.set(0, Led::Button,
color, Brightness::Lower) with leds.set(1, Led::Button, color,
Brightness::Lower)) while leaving storage.load_from_scene, glob_lfo_speed.set,
glob_div.set and get_color_for unchanged.
- Around line 407-412: The branch that handles chan == 0 assigns a new
destination to dest via storage.modify_and_save but never uses it to update the
button LED; modify the chan == 0 block so after computing dest (from
storage.modify_and_save on s.dest) you call the same LED-update routine used
elsewhere (match/map dest to the proper color/intensity and call the button LED
setter used in the LED update logic around the existing code) to reflect the new
destination on button0; reference the same functions/variables: chan,
storage.modify_and_save, s.dest and the button LED setter used in the LED update
section to implement the LED change.
- Around line 82-91: from_values currently indexes values[0..5] without
validating length which can panic; add a guard at the start of from_values that
returns None if values.len() < PARAMS (where PARAMS == 6) before any values[...]
accesses, then proceed to construct Self using usize::from_value(values[0]),
Range::from_value(values[1]), MidiOut::from_value(values[2]),
MidiChannel::from_value(values[3]), MidiCc::from_value(values[4]), and
Color::from_value(values[5]).
🧹 Nitpick comments (2)
faderpunk/src/apps/lfo_plus.rs (2)
203-203: Remove unnecessary async from closure.The
time_calcclosure is markedasyncbut doesn't contain any.awaitpoints. This adds unnecessary overhead.♻️ Simplify by removing async
- let time_calc = async |offset: u16| { + let time_calc = |offset: u16| { glob_lfo_speed.set( (curve.at(storage.query(|s| s.layer_speed)) as f32 + (offset) as f32 - 2047.0) * 0.015 + 0.0682,And update the call site:
let speed_offset = if destination == 0 { in_val } else { 2047 }; - time_calc(speed_offset).await; + time_calc(speed_offset);
276-281: Consider adding a comment for MIDI rate limiting logic.The division by 32 effectively rate-limits MIDI CC messages to only send when the value changes by approximately 0.78% (32/4096). This prevents excessive MIDI traffic but may not be immediately obvious to future maintainers.
📝 Suggested comment
output.set_value(val); if midi_out.is_some() { + // Rate limit MIDI CC: only send when value changes by at least 32 (≈0.78%) if last_out / 32 != val / 32 { midi.send_cc(midi_cc, val).await; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
configurator/src/components/ManualTab.tsxfaderpunk/src/apps/lfo_plus.rsfaderpunk/src/apps/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- faderpunk/src/apps/mod.rs
- configurator/src/components/ManualTab.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Firmware (Check)
🔇 Additional comments (5)
faderpunk/src/apps/lfo_plus.rs (5)
1-57: LGTM! Clean configuration setup.The imports, constants, and configuration are well-structured. The LFO+ app is properly configured with appropriate parameters for speed, range, MIDI output, and color customization.
105-130: LGTM! Well-documented storage structure.The storage struct is properly set up with sensible defaults and clear documentation for the
destfield values.
132-152: LGTM! Proper async task setup.The wrapper function correctly initializes stores, loads persistent state, and coordinates the main event loop with parameter handling and storage persistence.
469-480: LGTM! Proper concurrent task coordination.All event handlers are correctly joined to run concurrently using Embassy's join utilities.
482-490: LGTM! Clean waveform-to-color mapping.The helper function provides a clear, exhaustive mapping from waveforms to LED colors.
| fn from_values(values: &[Value]) -> Option<Self> { | ||
| Some(Self { | ||
| speed_mult: usize::from_value(values[0]), | ||
| range: Range::from_value(values[1]), | ||
| midi_out: MidiOut::from_value(values[2]), | ||
| midi_channel: MidiChannel::from_value(values[3]), | ||
| midi_cc: MidiCc::from_value(values[4]), | ||
| color_in: Color::from_value(values[5]), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for AppParams trait definition
rg -n 'trait AppParams' --type=rust
# Look for PARAMS constant
rg -n 'APP_MAX_PARAMS\|PARAMS' --type=rust | head -20Repository: ATOVproject/faderpunk
Length of output: 156
🏁 Script executed:
# Find the lfo_plus.rs file and examine the context around lines 82-91
fd lfo_plus.rs --type=rustRepository: ATOVproject/faderpunk
Length of output: 236
🏁 Script executed:
# Get more context about the from_values method and its usage
rg -n 'fn from_values' --type=rust -B2 -A5Repository: ATOVproject/faderpunk
Length of output: 10627
🏁 Script executed:
# Check for PARAMS constant in lfo_plus.rs
grep -n 'const PARAMS' faderpunk/src/apps/lfo_plus.rsRepository: ATOVproject/faderpunk
Length of output: 96
🏁 Script executed:
# Check the full Params struct and implementation in lfo_plus.rs
sed -n '50,110p' faderpunk/src/apps/lfo_plus.rsRepository: ATOVproject/faderpunk
Length of output: 1679
Add bounds check before array indexing in from_values.
The method indexes into the values array without validating its length. If values.len() < PARAMS (6), this will panic. All other app implementations check bounds before indexing—add the same validation:
if values.len() < PARAMS {
return None;
}
The framework does not guarantee the values array contains the required number of elements; the implementation must validate it.
🤖 Prompt for AI Agents
In @faderpunk/src/apps/lfo_plus.rs around lines 82 - 91, from_values currently
indexes values[0..5] without validating length which can panic; add a guard at
the start of from_values that returns None if values.len() < PARAMS (where
PARAMS == 6) before any values[...] accesses, then proceed to construct Self
using usize::from_value(values[0]), Range::from_value(values[1]),
MidiOut::from_value(values[2]), MidiChannel::from_value(values[3]),
MidiCc::from_value(values[4]), and Color::from_value(values[5]).
| let time_calc = async |offset: u16| { | ||
| glob_lfo_speed.set( | ||
| (curve.at(storage.query(|s| s.layer_speed)) as f32 + (offset) as f32 - 2047.0) * 0.015 | ||
| + 0.0682, | ||
| ); | ||
|
|
||
| let div = resolution | ||
| [((storage.query(|s| s.layer_speed) + offset - 2047) as usize / 500).clamp(0, 8)]; | ||
| glob_quant_speed.set(4095. / ((glob_count.get().max(1) as f32 * div as f32) / 24.)); | ||
| }; |
There was a problem hiding this comment.
Fix potential integer overflow in speed calculation.
Line 210 performs arithmetic on two u16 values without overflow protection:
((storage.query(|s| s.layer_speed) + offset - 2047) as usize / 500)When layer_speed (u16, max 65535) and offset (u16, max 65535) are added, the result can overflow u16. This will panic in debug builds or wrap in release builds, leading to incorrect speed calculations.
🔧 Proposed fix using checked or saturating arithmetic
let div = resolution
- [((storage.query(|s| s.layer_speed) + offset - 2047) as usize / 500).clamp(0, 8)];
+ [((storage.query(|s| s.layer_speed).saturating_add(offset).saturating_sub(2047)) as usize / 500).clamp(0, 8)];Alternatively, cast to a larger type before arithmetic:
let div = resolution
- [((storage.query(|s| s.layer_speed) + offset - 2047) as usize / 500).clamp(0, 8)];
+ [(((storage.query(|s| s.layer_speed) as i32 + offset as i32 - 2047).max(0)) as usize / 500).clamp(0, 8)];🤖 Prompt for AI Agents
In @faderpunk/src/apps/lfo_plus.rs around lines 203 - 212, The closure time_calc
performs arithmetic on two u16 values that can overflow when adding
storage.query(|s| s.layer_speed) and offset; change the computation to use a
wider integer or saturating/checked arithmetic before subtracting 2047 and
indexing resolution: e.g. cast both layer_speed and offset to u32 (or use
.saturating_add) then subtract 2047_i32/u32 safely, clamp the result to the
valid range, convert to usize and divide by 500 to compute the index; update the
expression that computes div (the resolution[...] index) and any subsequent
casts so all intermediate arithmetic happens in the larger type to avoid u16
overflow.
| if chan == 0 { | ||
| let dest = storage.modify_and_save(|s| { | ||
| s.dest = (s.dest + 1) % 3; | ||
| s.dest | ||
| }); | ||
| } |
There was a problem hiding this comment.
Unused variable suggests missing LED update logic.
The dest variable is assigned but never used. Based on the LED update logic in lines 316-322, you likely intended to update the button LED to reflect the new destination when shift+button0 is pressed.
💡 Suggested fix to update LED based on destination
if chan == 0 {
let dest = storage.modify_and_save(|s| {
s.dest = (s.dest + 1) % 3;
s.dest
});
+ let dest_color = match dest {
+ 0 => Color::Yellow,
+ 1 => Color::Pink,
+ 2 => Color::Cyan,
+ _ => Color::Yellow,
+ };
+ leds.set(0, Led::Button, dest_color, Brightness::Lower);
}📝 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 chan == 0 { | |
| let dest = storage.modify_and_save(|s| { | |
| s.dest = (s.dest + 1) % 3; | |
| s.dest | |
| }); | |
| } | |
| if chan == 0 { | |
| let dest = storage.modify_and_save(|s| { | |
| s.dest = (s.dest + 1) % 3; | |
| s.dest | |
| }); | |
| let dest_color = match dest { | |
| 0 => Color::Yellow, | |
| 1 => Color::Pink, | |
| 2 => Color::Cyan, | |
| _ => Color::Yellow, | |
| }; | |
| leds.set(0, Led::Button, dest_color, Brightness::Lower); | |
| } |
🤖 Prompt for AI Agents
In @faderpunk/src/apps/lfo_plus.rs around lines 407 - 412, The branch that
handles chan == 0 assigns a new destination to dest via storage.modify_and_save
but never uses it to update the button LED; modify the chan == 0 block so after
computing dest (from storage.modify_and_save on s.dest) you call the same
LED-update routine used elsewhere (match/map dest to the proper color/intensity
and call the button LED setter used in the LED update logic around the existing
code) to reflect the new destination on button0; reference the same
functions/variables: chan, storage.modify_and_save, s.dest and the button LED
setter used in the LED update section to implement the LED change.
| if shift { | ||
| let clocked = storage.modify_and_save(|s| { | ||
| s.clocked = !s.clocked; | ||
| s.clocked | ||
| }); | ||
| if clocked { | ||
| leds.set_mode(1, Led::Button, LedMode::Flash(color, Some(4))); | ||
| } |
There was a problem hiding this comment.
LED flash uses stale color that doesn't reflect current waveform.
Line 430 uses the color variable captured from line 187, which reflects the initial waveform. However, the waveform can change via button presses (line 399), causing the LED flash color to be inconsistent with the current waveform.
🎨 Proposed fix to use current waveform color
if shift {
let clocked = storage.modify_and_save(|s| {
s.clocked = !s.clocked;
s.clocked
});
if clocked {
+ let current_wave = storage.query(|s| s.wave);
+ let current_color = get_color_for(current_wave);
- leds.set_mode(1, Led::Button, LedMode::Flash(color, Some(4)));
+ leds.set_mode(1, Led::Button, LedMode::Flash(current_color, Some(4)));
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @faderpunk/src/apps/lfo_plus.rs around lines 424 - 431, The LED flash uses
the previously captured `color` instead of the current waveform color; after
toggling `s.clocked` in `storage.modify_and_save` (or immediately after that
call), read the current waveform/color from the storage state (e.g.,
`s.waveform` or a helper like `waveform_color(&s.waveform)`) and pass that
computed color into `leds.set_mode(1, Led::Button, LedMode::Flash(current_color,
Some(4)))` so the flash matches the updated waveform.
| SceneEvent::LoadSscene(scene) => { | ||
| storage.load_from_scene(scene).await; | ||
| let speed = storage.query(|s| s.layer_speed); | ||
| let wave_saved = storage.query(|s| s.wave); | ||
|
|
||
| glob_lfo_speed.set(curve.at(speed) as f32 * 0.015 + 0.0682); | ||
| glob_div.set(resolution[(speed as usize / 500).clamp(0, 8)]); | ||
|
|
||
| let color = get_color_for(wave_saved); | ||
| leds.set(0, Led::Button, color, Brightness::Lower); | ||
| } |
There was a problem hiding this comment.
Fix typo in match pattern and incorrect LED channel.
Two issues in the scene handler:
- Line 453: "LoadSscene" has a typo (double 's')
- Line 462: Sets LED 0 button to the waveform color, but LED 0 is used for input mute/destination throughout the code. The waveform button is LED 1 (see lines 189, 292, 294, 404).
🔧 Proposed fixes
match app.wait_for_scene_event().await {
- SceneEvent::LoadSscene(scene) => {
+ SceneEvent::LoadScene(scene) => {
storage.load_from_scene(scene).await;
let speed = storage.query(|s| s.layer_speed);
let wave_saved = storage.query(|s| s.wave);
glob_lfo_speed.set(curve.at(speed) as f32 * 0.015 + 0.0682);
glob_div.set(resolution[(speed as usize / 500).clamp(0, 8)]);
let color = get_color_for(wave_saved);
- leds.set(0, Led::Button, color, Brightness::Lower);
+ leds.set(1, Led::Button, color, Brightness::Lower);
}📝 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.
| SceneEvent::LoadSscene(scene) => { | |
| storage.load_from_scene(scene).await; | |
| let speed = storage.query(|s| s.layer_speed); | |
| let wave_saved = storage.query(|s| s.wave); | |
| glob_lfo_speed.set(curve.at(speed) as f32 * 0.015 + 0.0682); | |
| glob_div.set(resolution[(speed as usize / 500).clamp(0, 8)]); | |
| let color = get_color_for(wave_saved); | |
| leds.set(0, Led::Button, color, Brightness::Lower); | |
| } | |
| SceneEvent::LoadScene(scene) => { | |
| storage.load_from_scene(scene).await; | |
| let speed = storage.query(|s| s.layer_speed); | |
| let wave_saved = storage.query(|s| s.wave); | |
| glob_lfo_speed.set(curve.at(speed) as f32 * 0.015 + 0.0682); | |
| glob_div.set(resolution[(speed as usize / 500).clamp(0, 8)]); | |
| let color = get_color_for(wave_saved); | |
| leds.set(1, Led::Button, color, Brightness::Lower); | |
| } |
🤖 Prompt for AI Agents
In @faderpunk/src/apps/lfo_plus.rs around lines 453 - 463, The match arm uses
the misspelled variant SceneEvent::LoadSscene and writes the waveform color to
LED index 0; change the pattern to SceneEvent::LoadScene (matching the enum
variant) and update the leds.set call to target LED index 1 (i.e., replace
leds.set(0, Led::Button, color, Brightness::Lower) with leds.set(1, Led::Button,
color, Brightness::Lower)) while leaving storage.load_from_scene,
glob_lfo_speed.set, glob_div.set and get_color_for unchanged.
This is a new app:
LFO with assignable CV input