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.

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

feat(boards): add encoder support to planck #2155

Merged

Conversation

SethMilliken
Copy link
Contributor

This adds support for an optional rotary encoder in the leftmost column of the Planck rev6. All positions for the encoder map to the same pins on the board, so this ought to work regardless of which position is chosen.

I have been using this regularly for the last week without any issues. FWIW, my encoder is in the bottom row.

a-gpios = <&gpiob 12 GPIO_PULL_UP>;
b-gpios = <&gpiob 13 GPIO_PULL_UP>;
steps = <80>;
status = "okay";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There did not appear to be a clear precedent for setting the status of an encoder on a unibody board. Most boards set this to "disabled" in the dts and then set the respective encoder statuses to "okay" in the overlays for each half. Let me know if there is a better way to handle this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally you'd have it disabled, with a commented out section in the keymap to enable the encoder node if you're using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally you'd have it disabled, with a commented out section in the keymap to enable the encoder node if you're using it.

Cool. Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Encoder is now disabled by default with commented-out node in keymap to enable it.

SethMilliken pushed a commit to SethMilliken/zmk-config that referenced this pull request Feb 9, 2024
@@ -44,6 +44,20 @@
;
};

left_encoder: left_encoder {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the keyboard only supports one encoder, should we just call it "encoder"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had gone with that originally, but rev7 supports two encoders, one on each side. If we eventually support that, it seems like this would be a better fit if they share config. I can switch it back if you like though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whenever we do that, we can do that in a new planck_rev7.dts, etc. For now, keep this relevant to the rev6, so I'd suggest renaming back to just encoder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aye, aye. Renamed to encoder.

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@petejohanson petejohanson merged commit b44ec38 into zmkfirmware:main Feb 20, 2024
9 of 13 checks passed
@SethMilliken SethMilliken deleted the seth/planck-encoder-support branch February 20, 2024 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants