Improvements to Control #387
Conversation
chmanie
force-pushed
the
feat/control-improvements
branch
from
e78ea04 to
1da86f1
Compare
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThe control application extended its parameter configuration from 9 to 12 parameters by adding button-specific settings: button mode, MIDI channel, and MIDI CC. Runtime execution now includes conditional muted behavior based on button mode, a new button release handler task, and updated serialization paths to accommodate the new fields. Changes
Sequence DiagramsequenceDiagram
participant User as User<br/>(Button)
participant Handler as Button Handler
participant ModeSwitch as Mode Logic
participant LED as LED Control
participant MIDI as MIDI Output
participant RevHandler as Button Release<br/>Handler
User->>Handler: Button Press
Handler->>ModeSwitch: Check button_mode
alt Mode 0: Mute
ModeSwitch->>Handler: Use default behavior
Handler->>LED: Toggle LED
else Mode 1: CC Toggle
ModeSwitch->>Handler: Toggle mode
Handler->>LED: Update LED
Handler->>MIDI: Send CC (button_ch, button_cc)
else Mode 2: CC Momentary
ModeSwitch->>Handler: Momentary press
Handler->>LED: Light LED
Handler->>MIDI: Send CC ON (button_ch, button_cc)
Note over User,RevHandler: Button held...
User->>RevHandler: Button Release
RevHandler->>LED: Clear LED
RevHandler->>MIDI: Send CC OFF (button_ch, button_cc)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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: 1
🤖 Fix all issues with AI Agents
In @faderpunk/src/apps/control.rs:
- Around line 118-121: The deserializer now expects three new fields
(button_mode, button_ch, button_cc) before midi_out which breaks older 9-value
payloads; update the deserialization logic that processes values (the code using
values[...] to set button_mode, button_ch, button_cc, midi_out) to handle both
old and new formats by: check if values.len() == 9 then deserialize using the
old order, set button_mode, button_ch, button_cc to sensible defaults (e.g.,
default enum variants or 0) and map the old midi_out value accordingly;
otherwise when values.len() >= 12 parse the new fields as currently implemented.
Also add an optional version field or schema marker to the stored format for
future migrations and adjust the same deserialization path to prefer the
explicit version when present.
🧹 Nitpick comments (4)
faderpunk/src/apps/control.rs (4)
2-2: Remove unusedjoin4import.The code now uses
join5(line 460), making thejoin4import unnecessary.🔎 Proposed cleanup
- join::{join4, join5}, + join::join5,
289-293: Consider clarifying muted state behavior in CC modes.When
button_mode != 0(CC toggle/momentary modes), themutedstate is forced tofalsefor output calculations, but the button handler (lines 382-386) still toggles and persists themutedflag in storage. This creates a hidden state that only affects behavior when switching back to Mute mode.While this may be intentional to preserve user preference, consider either:
- Adding a comment explaining this design decision
- Only toggling/saving muted state when
button_mode == 0to make the behavior more explicit
405-418: Consider efficiency:rev_button_handlerruns even when not needed.When
button_mode != 2, this task waits for button events but performs no action (lines 413-416 are skipped). This wastes CPU cycles processing button events unnecessarily.Consider one of these approaches:
- Conditionally spawn
rev_button_handleronly whenbutton_mode == 2- Use
selectto allow early termination when parameters change- Move the
button_mode == 2check before the event wait🔎 Suggested optimization
let rev_button_handler = async { loop { + if button_mode != 2 { + // Skip unnecessary event processing in non-momentary modes + embassy_futures::yield_now().await; + continue; + } + if !on_release { buttons.wait_for_up(0).await; } else { buttons.wait_for_down(0).await; } - - if button_mode == 2 { - leds.unset(0, Led::Button); - midi_button.send_cc(button_cc, 0).await; - } + leds.unset(0, Led::Button); + midi_button.send_cc(button_cc, 0).await; } };Note: This assumes
button_modedoesn't change at runtime. If it does, a more sophisticated approach usingselectwould be needed.
443-454: Scene loading doesn't synchronize MIDI state in CC toggle mode.When loading a scene with
save_stateenabled, the LED is updated to reflect the storedmutedstate (lines 448-452), but no corresponding MIDI CC message is sent. In CC toggle mode (button_mode == 1), this creates a mismatch: the LED shows the loaded state, but external MIDI devices don't receive the update.Consider sending the appropriate CC value when loading scenes in mode 1:
🔎 Suggested fix for scene MIDI sync
SceneEvent::LoadSscene(scene) => { storage.load_from_scene(scene).await; if save_state { let muted = storage.query(|s| s.muted); muted_glob.set(muted); - if muted { - leds.unset(0, Led::Button); - } else { - leds.set(0, Led::Button, led_color, Brightness::Lower); + if button_mode == 1 { + // Sync MIDI state in CC toggle mode + if muted { + leds.unset(0, Led::Button); + midi_button.send_cc(button_cc, 0).await; + } else { + leds.set(0, Led::Button, led_color, Brightness::Lower); + midi_button.send_cc(button_cc, 4095).await; + } + } else if button_mode == 0 { + // Original mute mode behavior + if muted { + leds.unset(0, Led::Button); + } else { + leds.set(0, Led::Button, led_color, Brightness::Lower); + } } } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
faderpunk/src/apps/control.rs
🧰 Additional context used
🧬 Code graph analysis (1)
faderpunk/src/apps/control.rs (1)
libfp/src/lib.rs (31)
default(122-125)default(988-990)default(1010-1012)default(1103-1105)from(176-183)from(222-238)from(575-577)from(581-601)from(623-631)from(732-734)from(738-740)from(744-746)from(750-752)from(756-758)from(762-764)from(768-770)from(774-776)from(780-782)from(786-788)from(792-794)from_value(242-247)from_value(483-488)from_value(525-530)from_value(605-610)from_value(943-948)from_value(955-960)from_value(979-984)from_value(1016-1021)from_value(1038-1043)from_value(1081-1086)from_value(1094-1099)
⏰ 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 (2)
faderpunk/src/apps/control.rs (2)
387-402: LGTM: Button mode logic correctly implements toggle and momentary behaviors.The conditional logic properly handles:
- Mode 0/1: LED state follows the toggled muted flag
- Mode 1: Additional CC messages sent based on state
- Mode 2: Momentary press handled (release handled in
rev_button_handler)
460-467: LGTM: Correctly updated tojoin5for the additional concurrent task.The five concurrent tasks are properly integrated:
main_loop- Output calculation and LED updatesbutton_handler- Button press/release handlingrev_button_handler- Complementary button event handler for momentary modefader_event_handler- Fader change processingscene_handler- Scene load/save operations
chmanie
left a comment
chmanie
left a comment
There was a problem hiding this comment.
Should be great! Only one comment here
| } | ||
| } | ||
| }; | ||
| let rev_button_handler = async { |
There was a problem hiding this comment.
I would probably only initialize one of the two handlers, based on the on_release param, then you can save one future.
There was a problem hiding this comment.
I won't be able to do this till next week unfortunately. Feel free to edit the commit if you want.
Add param to change button behavior.