Added code to handle Half-step encoders along with a config option - #246
Added code to handle Half-step encoders along with a config option#246Kflin01 wants to merge 2 commits into
Conversation
to turn it on/off. Have a mixture of Quad and Half step encoders and wanted an easy way to use either without recompiling.
📝 WalkthroughWalkthroughThis adds a configurable "HalfStep" mode to the rotary encoder usermod, updates encoder edge/level decoding to support half-step and full-step behaviors, and persists the new mode in configuration storage. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RotaryEncoder
participant RotaryEncoderUIUsermod
participant ConfigStorage
User ->> RotaryEncoder: rotate knob
RotaryEncoder ->> RotaryEncoderUIUsermod: pin A/B state changes
RotaryEncoderUIUsermod ->> RotaryEncoderUIUsermod: sample Enc_A/Enc_B and prev states
RotaryEncoderUIUsermod ->> RotaryEncoderUIUsermod: check HalfStep flag
alt HalfStep mode
RotaryEncoderUIUsermod ->> RotaryEncoderUIUsermod: on A or B change, evaluate combined A/B to determine direction
else FullStep mode
RotaryEncoderUIUsermod ->> RotaryEncoderUIUsermod: on falling edge of A, read B to determine direction
end
RotaryEncoderUIUsermod ->> ConfigStorage: read/write HalfStep flag
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h (1)
640-640: Remove misleading comment.The comment "kkf check for half steps" is confusing as it appears in the full-step code branch.
-// kkf check for half steps
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h(9 hunks)
🔇 Additional comments (4)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h (4)
152-152: LGTM! Member variables properly added for half-step support.The additions of
Enc_B_prev,HalfStep, and_HalfStepare correct and necessary for implementing the half-step encoder functionality.Also applies to: 166-166, 180-180
336-336: String constant properly defined.The PROGMEM string definition follows the established pattern for configuration keys.
469-469: Encoder pin B state properly initialized.Initializing
Enc_B_prevalong withEnc_A_prevensures proper edge detection from the start.
1141-1141: Configuration reading properly implemented.The
HalfStepconfiguration value is correctly read using the established pattern.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
softhack007
commented
Apr 6, 2026
@coderabbitai re-review please. |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h (1)
1141-1168:⚠️ Potential issue | 🟡 MinorUse
_HalfStepas the config-completeness sentinel.Line 1167 already documents the pattern for new settings, but the return value still keys off
_applyToAll. With the current code, upgraded configs that are missing only_HalfStepwill be reported as complete, sowled00/um_manager.cpp:31-37andwled00/cfg.cpp:633-645never get thefalsethey need to backfill the new option intocfg.json.Suggested fix
- // use "return !top["newestParameter"].isNull();" when updating Usermod with new features- return !top[FPSTR(_applyToAll)].isNull();+ // use newest parameter so older cfg.json files get backfilled once+ return !top[FPSTR(_HalfStep)].isNull();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h` around lines 1141 - 1168, The config-completeness sentinel should be the new setting key `_HalfStep` rather than `_applyToAll`; update the final return in the Usermod config loader to check for the presence of FPSTR(_HalfStep) (i.e., return !top[FPSTR(_HalfStep)].isNull()) so missing `_HalfStep` triggers the backfill logic in um_manager/cfg rather than incorrectly reporting the config as complete.
🧹 Nitpick comments (1)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h (1)
593-680: Consider collapsing the repeated action switch into one helper.This decoder now maintains four copies of the same
switch(select_state)body. Extracting something likeapplyEncoderStep(bool increase)would keep this block focused on direction detection and make future fixes much less error-prone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h` around lines 593 - 680, The switch(select_state) block is duplicated four times; extract it into a single helper like applyEncoderStep(bool increase) (or applyEncoderStep(int delta)) that contains the switch calling changeBrightness, changeEffectSpeed, changeEffectIntensity, changePalette, changeEffect, changeHue, changeSat, changeCCT, changePreset and changeCustom(1/2/3) based on select_state, then replace each duplicated switch in the HalfStep and Full Step branches with a single call to applyEncoderStep(true) or applyEncoderStep(false) (or applyEncoderStep(+1)/applyEncoderStep(-1)) so direction detection logic (Enc_A/Enc_B, Enc_A_prev/Enc_B_prev, HalfStep) remains separate from action dispatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h`:
- Around line 639-644: Restore the legacy full-step rising-edge trigger for
non-half-step encoders: use the original check (Enc_A transitions from LOW to
HIGH, i.e., (Enc_A && !Enc_A_prev)) when the encoder is not in HalfStep mode,
and only use the falling-edge detection ((Enc_A_prev == HIGH) && (Enc_A == LOW))
for HalfStep operation; ensure the Enc_B direction test remains the same in each
branch so existing non-HalfStep clockwise/counter-clockwise behavior is
preserved (refer to symbols Enc_A, Enc_A_prev, Enc_B, and HalfStep to locate and
adjust the logic).
---
Outside diff comments:
In
`@usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h`:
- Around line 1141-1168: The config-completeness sentinel should be the new
setting key `_HalfStep` rather than `_applyToAll`; update the final return in
the Usermod config loader to check for the presence of FPSTR(_HalfStep) (i.e.,
return !top[FPSTR(_HalfStep)].isNull()) so missing `_HalfStep` triggers the
backfill logic in um_manager/cfg rather than incorrectly reporting the config as
complete.
---
Nitpick comments:
In
`@usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h`:
- Around line 593-680: The switch(select_state) block is duplicated four times;
extract it into a single helper like applyEncoderStep(bool increase) (or
applyEncoderStep(int delta)) that contains the switch calling changeBrightness,
changeEffectSpeed, changeEffectIntensity, changePalette, changeEffect,
changeHue, changeSat, changeCCT, changePreset and changeCustom(1/2/3) based on
select_state, then replace each duplicated switch in the HalfStep and Full Step
branches with a single call to applyEncoderStep(true) or applyEncoderStep(false)
(or applyEncoderStep(+1)/applyEncoderStep(-1)) so direction detection logic
(Enc_A/Enc_B, Enc_A_prev/Enc_B_prev, HalfStep) remains separate from action
dispatch.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d31058a-25bb-4fd7-8122-9c2c72b4c550
📒 Files selected for processing (1)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h
| // if ((Enc_A) && (!Enc_A_prev)) | ||
| // kkf check for half steps | ||
| if ((Enc_A_prev == HIGH) && (Enc_A == LOW)) | ||
| { // WLEDMM: use falling edge detection | ||
| if (Enc_B == HIGH) | ||
| { |
There was a problem hiding this comment.
Preserve the legacy full-step edge here.
The commented-out check on Line 639 is the old rising-edge trigger. Changing Line 641 to the falling edge while keeping the same Enc_B direction test flips clockwise/counter-clockwise for existing non-HalfStep encoders, which is a regression in the compatibility path.
Suggested fix
- if ((Enc_A_prev == HIGH) && (Enc_A == LOW))- { // WLEDMM: use falling edge detection+ if ((Enc_A_prev == LOW) && (Enc_A == HIGH))+ { // preserve legacy full-step behavior📝 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 ((Enc_A) && (!Enc_A_prev)) | |
| // kkf check for half steps | |
| if ((Enc_A_prev==HIGH) && (Enc_A==LOW)) | |
| { // WLEDMM: use falling edge detection | |
| if (Enc_B==HIGH) | |
| { | |
| // if ((Enc_A) && (!Enc_A_prev)) | |
| // kkf check for half steps | |
| if ((Enc_A_prev==LOW) && (Enc_A==HIGH)) | |
| { // preserve legacy full-step behavior | |
| if (Enc_B==HIGH) | |
| { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h`
around lines 639 - 644, Restore the legacy full-step rising-edge trigger for
non-half-step encoders: use the original check (Enc_A transitions from LOW to
HIGH, i.e., (Enc_A && !Enc_A_prev)) when the encoder is not in HalfStep mode,
and only use the falling-edge detection ((Enc_A_prev == HIGH) && (Enc_A == LOW))
for HalfStep operation; ensure the Enc_B direction test remains the same in each
branch so existing non-HalfStep clockwise/counter-clockwise behavior is
preserved (refer to symbols Enc_A, Enc_A_prev, Enc_B, and HalfStep to locate and
adjust the logic).
to turn it on/off. Have a mixture of Quad and Half step encoders and wanted an easy way to use either without recompiling.
Summary by CodeRabbit
New Features
Bug Fixes