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

Encoder Support in CLI qmk compile #8440

Closed
wants to merge 33 commits into from

Conversation

xplusplus
Copy link
Contributor

@xplusplus xplusplus commented Mar 16, 2020

Description

This is a PR to add encoder support to the QMK CLI compile command. The option does not modify current compilation behavior in the CLI.

My intent is for this change to serve as a proof of concept for adding this behavior to the configurator and compiler. There is an open thread in the qmk_configurator repo: qmk/qmk_configurator#468

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@Erovia
Copy link
Member

Erovia commented Mar 16, 2020

It seems that this PR is based on an older state of the master branch.
Could you please rebase it on the current HEAD and resolve the merge conflicts?

@Erovia Erovia requested a review from yanfali March 16, 2020 10:23
@xplusplus
Copy link
Contributor Author

Updated to fork to latest.

lib/python/qmk/keymap.py Outdated Show resolved Hide resolved
Copy link
Member

@skullydazed skullydazed left a comment

Choose a reason for hiding this comment

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

I haven't looked at the logic too carefully yet but I wanted to provide some style guidance here.

lib/python/qmk/cli/compile.py Outdated Show resolved Hide resolved
lib/python/qmk/cli/compile.py Outdated Show resolved Hide resolved
lib/python/qmk/commands.py Outdated Show resolved Hide resolved
lib/python/qmk/keymap.py Outdated Show resolved Hide resolved
lib/python/qmk/keymap.py Outdated Show resolved Hide resolved
lib/python/qmk/keymap.py Outdated Show resolved Hide resolved
lib/python/qmk/keymap.py Outdated Show resolved Hide resolved
lib/python/qmk/keymap.py Outdated Show resolved Hide resolved
@xplusplus xplusplus marked this pull request as ready for review March 21, 2020 05:33
@fauxpark
Copy link
Member

fauxpark commented Mar 22, 2020

Do we want to include support for things like the RGB and backlight inc/dec keycodes? If so, there'll need to be some special handling for them since passing them to tap_code() won't work. I also think tap_code16() might be better so as to support modded keycodes as well.

EDIT: In fact... there needs to be some sanity checking either way to handle the case where a user inputs a Quantum keycode here, either by placing a KC_NO instead, or erroring out gracefully - GCC will complain if you try to stuff a uint16 into tap_code()'s uint8 argument.

@xplusplus
Copy link
Contributor Author

@fauxpark Updated to use tap_code16 as well as do a basic check for 'basic' qmk keycodes. Since I eventually want this to be a part of the configurator, I'll need to find a solution for "advanced' codes. But for now, I think the basic codes are enough functionality for most users.

@xplusplus
Copy link
Contributor Author

CI/CD being affected by #8533. On hold until that is merged.

@zvecr
Copy link
Member

zvecr commented Mar 24, 2020

As mentioned on Discord, there is no hard requirement that all keymaps build against all revisions. #8533 is unlikely to fix every keymap combo, and given the current scope of this RP, will not be blocking this from being merged.

That aside, i'm not a fan of the onekey/pytest2 testing strategy used here. Feels.... messy.

keyboards/handwired/onekey/encodertest/encoder.json Outdated Show resolved Hide resolved
keyboards/handwired/onekey/encodertest/encoder.json Outdated Show resolved Hide resolved
keyboards/handwired/onekey/encodertest/encoder.json Outdated Show resolved Hide resolved
keyboards/handwired/onekey/encodertest/no_encoder.json Outdated Show resolved Hide resolved
keyboards/handwired/onekey/encodertest/no_encoder.json Outdated Show resolved Hide resolved
lib/python/qmk/keymap.py Outdated Show resolved Hide resolved
lib/python/qmk/keymap.py Outdated Show resolved Hide resolved
lib/python/qmk/keymap.py Outdated Show resolved Hide resolved
lib/python/qmk/keymap.py Outdated Show resolved Hide resolved
lib/python/qmk/keymap.py Show resolved Hide resolved
Copy link
Member

@Erovia Erovia left a comment

Choose a reason for hiding this comment

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

Approved.

Would like to get @skullydazed's feedback on it before merging.

keyboards/handwired/onekey/keymaps/encodertest/keymap.json Outdated Show resolved Hide resolved
Comment on lines +27 to +34
ENCODER_IF = """\t_ELIF_ (index == _INDEX_) {
\t\tif (clockwise) {
\t\t\ttap_code16(_CLOCKWISE_);
\t\t} else {
\t\t\ttap_code16(_COUNTER_);
\t\t}
\t}"""

Copy link
Member

Choose a reason for hiding this comment

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

As per my previous comment, this will need some logic with some checks to see what layer we're currently in and using the appropriate code.

@tzarc tzarc requested a review from a team November 9, 2020 20:52
Comment on lines +36 to +42
ENCODER_IF = """\t_ELIF_ (index == _INDEX_) {
\t\tif (clockwise) {
\t\t\ttap_code16(_CLOCKWISE_);
\t\t} else {
\t\t\ttap_code16(_COUNTER_);
\t\t}
\t}"""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ENCODER_IF = """\t_ELIF_ (index == _INDEX_) {
\t\tif (clockwise) {
\t\t\ttap_code16(_CLOCKWISE_);
\t\t} else {
\t\t\ttap_code16(_COUNTER_);
\t\t}
\t}"""
ENCODER_IF = """ _ELIF_ (index == _INDEX_) {
if (clockwise) {
tap_code16(_CLOCKWISE_);
} else {
tap_code16(_COUNTER_);
}
}"""

@xplusplus
Copy link
Contributor Author

alright this pr got too stale and i'm having environmental issues. I'm going to take the feedback and start again.

@xplusplus xplusplus closed this Dec 16, 2020
@skullydazed
Copy link
Member

While you're doing that you may want to check out #10817, #11101, and #11108 which flesh out the keyboard json support a lot. Next I'll be building on that for keymaps, and that will make some of this encoder work more relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants