Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix rotary encoder handling - resurrection of #730 #4202

Closed
wants to merge 7 commits into from
Closed

Fix rotary encoder handling - resurrection of #730 #4202

wants to merge 7 commits into from

Conversation

susisstrolch
Copy link

Original fix from @nickbrennan01

This feature supports half-stepping rotary encoders and encoders with inverted
inputs (like Velleman K8800).

Requires an additional config entry in [display] section:
encoder_steps_per_detent: 2|4

encoder_steps_per_detent = 2: "regular" encoder
encoder_steps_per_detent = 4: half-stepping encoder

Signed-off-by: Juergen Orschiedt juorschiedt@gmail.com

susisstrolch and others added 4 commits April 23, 2021 09:44
Fix rotary encoder handling - resurrection of Klipper3d#730
58c1b5b 
Original fix from @nickbrennan01

This feature supports half-stepping rotary encoders and encoders with inverted
inputs (like Velleman K8800).

Requires an additional config entry in [display] section:
  encoder_steps_per_detent: 2|4

encoder_steps_per_detent = 2: "regular" encoder
encoder_steps_per_detent = 4: half-stepping encoder
changed line with to fix build
67258a6 
Signed-off-by: Jelle Victoor <victoor.jelle@gmail.com>
updated config with steps_per_detent documentation
5cb221a 
Signed-off-by: Jelle Victoor <victoor.jelle@gmail.com>
changed debug logging to info logging
195e502 
Signed-off-by: Jelle Victoor <victoor.jelle@gmail.com>
@jellevictoor
Copy link
Contributor

updated with documentation

@susisstrolch
encoder_steps_per_detent: defaults to full step (2)
fd3204a 
We set the default to full step to stay compatible
with the old behaviour.

Signed-off-by: susisstrolch <susiconstrolch@gmail.com>
@KevinOConnor
Copy link
Collaborator

Thanks - some high-level comments:

  1. Please don't mix white space changes in with functional changes. It makes it harder to review the code and makes it harder to find regressions (should a regression be introduced). Also, FWIW, the white space changes wont be accepted.
  2. FWIW, I find mixing the different detent config in the the state machine to be confusing. Maybe using two different state machines would be simpler.

@mcmatrix - do you have any comments on this?

-Kevin

@susisstrolch
Copy link
Author

The white spacee changes were accidental - I've removed them.
So shall I push it again, after whitespace and line length are fixed?
Or shall I simply close the request, until anybody else comes up with a splitted state machine and simply let those with a half step encoder wait for the next volunteer who tries to solve the problem instead having them hanging around without working solution?

@KevinOConnor
Copy link
Collaborator

I'm not sure what the state of this PR is. Is there still interest in it?

-Kevin

@girrrrrrr2
Copy link

I'm not sure what the state of this PR is. Is there still interest in it?

I am interested in it, and I feel like it could be a great addition to klipper/3d printing as a whole, but I dont know if there are others that are also interested or if its just me, or if that comment was referenced to just the author...

@KevinOConnor
Copy link
Collaborator

I suspect this feature will need a developer to lead it to completion. Otherwise, I don't see it getting merged without a conclusion on the review comments and fixing the build failures.

-Kevin

Jelle Victoor added 2 commits June 17, 2021 20:16
whitespace check, fix build
988fd3b 
Signed-off-by: Jelle Victoor <victoor.jelle@gmail.com>
split up the rotary encoder into separate classes
68f8f9b 
Signed-off-by: Jelle Victoor <victoor.jelle@gmail.com>
@jellevictoor
Copy link
Contributor

Hi @KevinOConnor
I took in account the fixes you suggested and fixed the build. is this ok to merge?

@KevinOConnor
Copy link
Collaborator

Sorry, but when I look at the diff I still see the items I mentioned at #4202 (comment) . There are still unrelated white space changes. Also, I find the new state machine to be quite confusing (the new code seems to be mixing two different devices into one state machine).

-Kevin

@jellevictoor
Copy link
Contributor

Based on the comments of susisstrolch i thought the whitespaces were ok, I'll revisit
can you elaborate more on how you would split up the state machine? i have limited knowledge of python programming, i just applied the composition over inheritance, since this is not exposed to the outside, but i'm happy to take your suggestions so this could be merged

@KevinOConnor
Copy link
Collaborator

can you elaborate more on how you would split up the state machine?

I find it confusing to modify ENCODER_STATES; I would make an entirely new ENCODER_STATES_FOR_1_DETENT (or similar) and not modify the existing state machine.

Alas, due to time constraints on my time, this isn't something I can spend further time on.

-Kevin

@KevinOConnor KevinOConnor added the pending feedback Topic is pending feedback from submitter label Jul 15, 2021
@github-actions github-actions bot added the inactive Not currently being worked on label Aug 18, 2021
@github-actions
Copy link

It looks like this GitHub Pull Request has become inactive. If there are any further updates, you can add a comment here or open a new ticket.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

@github-actions github-actions bot closed this Aug 18, 2021
rufo added a commit to rufo/klipper that referenced this pull request Sep 12, 2021
@rufo
button: Support half-stepping encoders
a53b97e 
Adds support for half-stepping encoders (encoders that only emit two
steps per detent, instead of four). Incorporates the feedback from
@susisstrolch's PR: Klipper3d#4202
, which was itself built upon a previous PR from @nickbrennan01:
Klipper3d#730

Uses the table from the Rotary Arduino library linked in buttons.py:
https://github.com/brianlow/Rotary/blob/6b784cca67c5f1ce5e11d757a540fc4c0311efca/Rotary.cpp#L21-L40

Signed-off-by: Rufo Sanchez <rufo@rufosanchez.com>
@rufo rufo mentioned this pull request Sep 12, 2021
Merged
rufo added a commit to rufo/klipper that referenced this pull request Sep 12, 2021
@rufo
button: Support half-stepping encoders
35d91ce 
Adds support for half-stepping encoders (encoders that only emit two
steps per detent, instead of four). Incorporates the feedback from
@susisstrolch's PR: Klipper3d#4202
, which was itself built upon a previous PR from @nickbrennan01:
Klipper3d#730

Uses the table from the Rotary Arduino library linked in buttons.py:
https://github.com/brianlow/Rotary/blob/6b784cca67c5f1ce5e11d757a540fc4c0311efca/Rotary.cpp#L21-L40

Signed-off-by: Rufo Sanchez <rufo@rufosanchez.com>
rufo added a commit to rufo/klipper that referenced this pull request Oct 11, 2021
@rufo
button: Support half-stepping encoders
e860d64 
Adds support for half-stepping encoders (encoders that only emit two
steps per detent, instead of four). Incorporates the feedback from
@susisstrolch's PR: Klipper3d#4202
, which was itself built upon a previous PR from @nickbrennan01:
Klipper3d#730

Uses the table from the Rotary Arduino library linked in buttons.py:
https://github.com/brianlow/Rotary/blob/6b784cca67c5f1ce5e11d757a540fc4c0311efca/Rotary.cpp#L21-L40

Signed-off-by: Rufo Sanchez <rufo@rufosanchez.com>
KevinOConnor pushed a commit that referenced this pull request Oct 11, 2021
@rufo @KevinOConnor
button: Support half-stepping encoders
b336a21 
Adds support for half-stepping encoders (encoders that only emit two
steps per detent, instead of four). Incorporates the feedback from
@susisstrolch's PR: #4202
, which was itself built upon a previous PR from @nickbrennan01:
#730

Uses the table from the Rotary Arduino library linked in buttons.py:
https://github.com/brianlow/Rotary/blob/6b784cca67c5f1ce5e11d757a540fc4c0311efca/Rotary.cpp#L21-L40

Signed-off-by: Rufo Sanchez <rufo@rufosanchez.com>
troy-jacobson pushed a commit to troy-jacobson/klipper that referenced this pull request Nov 22, 2021
@rufo @troy-jacobson
button: Support half-stepping encoders
a6b21c2 
Adds support for half-stepping encoders (encoders that only emit two
steps per detent, instead of four). Incorporates the feedback from
@susisstrolch's PR: Klipper3d#4202
, which was itself built upon a previous PR from @nickbrennan01:
Klipper3d#730

Uses the table from the Rotary Arduino library linked in buttons.py:
https://github.com/brianlow/Rotary/blob/6b784cca67c5f1ce5e11d757a540fc4c0311efca/Rotary.cpp#L21-L40

Signed-off-by: Rufo Sanchez <rufo@rufosanchez.com>
@github-actions github-actions bot locked and limited conversation to collaborators Oct 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
inactive Not currently being worked on pending feedback Topic is pending feedback from submitter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@susisstrolch @jellevictoor @KevinOConnor @girrrrrrr2