-
Notifications
You must be signed in to change notification settings - Fork 116
Add support for half step and quarter step encoders #1008
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for half step and quarter step encoders #1008
Conversation
Allows configuring rotary encoder type via minidexed.ini. Supports quarter-step (1), half-step (2), and full-step (4) encoders. - Add EncoderDetents parameter to config (default=4) - Pass encoder detents to KY040 driver constructor - Document EncoderDetents option in minidexed.ini
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a configurable encoder detent count and wires it through configuration to the KY040 rotary encoder driver, updating the circle-stdlib submodule so MiniDexed can correctly support full-, half-, and quarter-step encoders while preserving current default behaviour. Sequence diagram for encoder initialization with detent configurationsequenceDiagram
participant UI as CUserInterface
participant CFG as CConfig
participant ENC as CKY040
UI->>CFG: GetEncoderEnabled()
CFG-->>UI: enabledFlag
alt encoder enabled
UI->>CFG: GetEncoderPinClock()
CFG-->>UI: pinClock
UI->>CFG: GetEncoderPinData()
CFG-->>UI: pinData
UI->>CFG: GetEncoderDetents()
CFG-->>UI: encoderDetents
UI->>ENC: new CKY040(pinClock, pinData, buttonPinShortcut, gpioManager, encoderDetents)
UI->>ENC: Initialize()
ENC-->>UI: initResult
end
Class diagram for configurable encoder detents supportclassDiagram
class CConfig {
- bool m_bEncoderEnabled
- unsigned m_nEncoderPinClock
- unsigned m_nEncoderPinData
- unsigned m_nEncoderDetents
+ void Load()
+ bool GetEncoderEnabled()
+ unsigned GetEncoderPinClock()
+ unsigned GetEncoderPinData()
+ unsigned GetEncoderDetents()
}
class CUserInterface {
- CConfig* m_pConfig
- CKY040* m_pRotaryEncoder
+ bool Initialize()
}
class CKY040 {
+ CKY040(unsigned pinClock, unsigned pinData, unsigned buttonPin, CGPIOManager* gpioManager, unsigned encoderDetents)
+ bool Initialize()
}
CUserInterface --> CConfig : uses
CUserInterface --> CKY040 : creates
CConfig ..> CKY040 : provides encoderDetents
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've left some high level feedback:
- Consider validating
EncoderDetentsinCConfig::Load(e.g. enforcing a minimum of 1 and optionally clamping to a small set of sensible values like 1/2/4) and logging a warning when an out-of-range value is provided, to avoid undefined behavior from misconfigured INI files. - The INI comment says "Detents per rotation to be recognized" while the parameter is named
EncoderDetents; if the driver actually expects detents per step or a similar notion, it may be worth tightening the wording to make the expected unit/semantics crystal clear to users.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider validating `EncoderDetents` in `CConfig::Load` (e.g. enforcing a minimum of 1 and optionally clamping to a small set of sensible values like 1/2/4) and logging a warning when an out-of-range value is provided, to avoid undefined behavior from misconfigured INI files.
- The INI comment says "Detents per rotation to be recognized" while the parameter is named `EncoderDetents`; if the driver actually expects detents per step or a similar notion, it may be worth tightening the wording to make the expected unit/semantics crystal clear to users.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
hi @speendo, thanks for this pull request. The build fails due to a constructor mismatch for CKY040 at this line in userinterface.cpp: m_pRotaryEncoder = new CKY040(
m_pConfig->GetEncoderPinClock(),
m_pConfig->GetEncoderPinData(),
m_pConfig->GetButtonPinShortcut(),
m_pGPIOManager,
m_pConfig->GetEncoderDetents()
);The error: and from ky040.h: Solution:
For example, if CKY040 only takes 3 arguments: m_pRotaryEncoder = new CKY040(
m_pConfig->GetEncoderPinClock(),
m_pConfig->GetEncoderPinData(),
m_pGPIOManager
);Adjust the arguments in userinterface.cpp's Initialize() method at lines 181–185 accordingly. Check CKY040's class declaration and documentation for correct usage. This change should resolve the constructor mismatch and allow your build to proceed. |
|
I love this AI world already! ;) Update the circle-stdlib to v17.2 and the circle to the latest develop commit, in submod.sh: If this PR gets merged, I'll also rebase the DreamDexed on it. |
- Replace numeric parameter with user-friendly strings: full, half, quarter - Add validation with silent fallback to default (full) - Clarify INI documentation Addresses feedback from sourcery-ai bot review.
|
Thanks everyone for the reviews and feedback! @sourcery-ai - I've addressed the validation feedback by changing from numeric EncoderDetents to a string-based EncoderResolution parameter with values "full" (default and fallback for invalid values), "half", or "quarter". I didn't implement the idea of logging invalid values as this wouldn't match the current style how @probonopd - The build failure is expected at this point. The PR depends on the new CKY040 constructor that was just merged to Circle's develop branch (rsta2/circle#683). The current MiniDexed submodules point to older Circle versions that still use the 4-parameter constructor. @soyersoyer - Thank you for testing and for the solution! I'm excited to hear DreamDexed is using this! The updated changes have just been pushed. Once the submodules are updated as soyersoyer suggested, the build should succeed. |
|
It's also worth mentioning that this change potentially renders capacitors on the encoder's CLK and DT pins obsolete (see the discussion here), so the building process could be slightly changed and simplified. Of course, keeping the capacitors does no harm. |
- Changed EncoderResolution parsing to use std::string with == operator - Removes dependency on strings.h header - Consistent with other string configuration options in the file
|
The build still fails. |
- Update circle-stdlib to v17.2 - Update circle to commit b42d060 which includes the new CKY040 constructor with encoder detents parameter - This fixes the build failure by providing the correct API for the encoder resolution feature
|
Build for testing: |
|
Thanks for checking, @soyersoyer! I thought it would be just a waiting game until the changes get merged to the main branches of circle-stdlib and circle. Changing submod.sh of course makes waiting obsolete :) Thanks! |
|
Thank you very much @speendo and @soyersoyer. Please let me know whether the build for testing (above) works for you and whether you think this is ready for being merged. Once merged, let's not forget to update the wiki. |
|
So I downloaded the automatic 64bit build to use on my Raspberry Pi 3. I can confirm that
I didn't test
From this biased perspective, I am rather optimistic that the build would be ready to be merged. One thing might still be worth discussing: Do you like the style of the new variable in I bring this up because this requires the user to know that there are different styles of encoders (full-step, half-step and quarter-step) and also which version they own. Another (rather naive) approach would be the user installing MiniDexed (which is defaulted to a full-step encoder) then noticing that the encoder needs 2 (or even 4) detents to recognise a step in the menu and then searching and editing a variable like What do you think is better? |
|
@speendo I like the variables as they are implemented now. We just need to document them in the Wiki once merged... which is... now :) Thank you very much and I'd appreciate if you could update the Wiki. Thanks! |
|
DreamDexed has been rebased on top of this. Thanks. |
|
Great! I updated several pages in the Wiki to reflect the changes. |
Summary
Adds a new
EncoderDetentsconfiguration option to support different rotary encoder hardware types, fixing compatibility issues with half-step and quarter-step encoders.Problem
MiniDexed uses the circle library to implement rotary encoders. This library only worked correctly with full-step encoders. Users with half-step encoders (increasingly common) experienced missed steps. This issue was discussed at
Solution
Changes
Configuration
minidexed.ini: AddedEncoderDetents=4with documentationconfig.h: Addedm_nEncoderDetentsmember and getterconfig.cpp: Added parsing forEncoderDetentsIntegration
userinterface.cpp: Pass encoder detents value to CKY040 constructorcircle-stdlib: Updated submodule for KY040 driver improvementsDependencies
This PR depends on:
Testing
Build verified on Raspberry Pi 3
Tested on a Raspberry Pi 3 with a half step encoder (HW-040)
EncoderDetents=2: works correctlyEncoderDetents=4: maintains existing behaviour (only every second step is recognised)EncoderDetents=1: two steps per detentBackward Compatibility
The default value (EncoderDetents=4) maintains existing behavior, so users with full-step encoders don't need to change anything.
Summary by Sourcery
Add configurable support for different rotary encoder detent resolutions to improve compatibility with various encoder hardware.
New Features:
Enhancements:
Documentation: