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

Asymmetric encoders, encoder tests. #16068

Merged
merged 12 commits into from
Mar 8, 2022

Conversation

tzarc
Copy link
Member

@tzarc tzarc commented Jan 27, 2022

Description

Previous asymmetric encoders PR was broken, and was rolled back shortly after merge.
In addition, encoders had tests, but weren't hooked up.

This PR attempts to address both situations -- enabling the existing test runs showed that encoder tests were broken, so I figured it was best to actually fix up encoder sizing assumptions at the same time.

Modifications

  • Encoder tests have been re-enabled
  • Encoder tests have been added to:
    • Initialisation now clears out the encoder state, fixing commented-out tests
    • Asymmetric tests have been added, with encoder counts: L=R, L<R, L>R, L=0, R=0.
  • Encoder implementation has been fixed up such that asymmetric encoders are usable
    • keymap.h has new defines to allow shorthand for encoder counts, as well as per-side
    • Split common has been amended to ensure only the required number of encoders are transmitted
    • Encoder resolutions weren't correctly being set up in keyboards -- we had ENCODER_RESOLUTIONS_RIGHT already documented but there were places where keyboards were using ENCODER_RESOLUTIONS and ENCODER_RESOLUTIONS_RIGHT incorrectly together (or not at all).
    • ENCODER_RESOLUTIONS is used for both left and right if ENCODER_RESOLUTIONS_RIGHT is unspecified, matching ENCODERS_PAD_A.

Types of Changes

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

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • 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).

@tzarc tzarc requested a review from a team January 27, 2022 01:18
@tzarc tzarc force-pushed the fix-encoders-and-enable-tests branch from 6a6d5e2 to 9b039d1 Compare January 27, 2022 05:15
@tzarc tzarc force-pushed the fix-encoders-and-enable-tests branch from d1618fd to d804133 Compare February 4, 2022 21:16
@tzarc tzarc marked this pull request as ready for review February 4, 2022 21:54
@drashna drashna requested a review from a team February 5, 2022 03:19
@mtei
Copy link
Contributor

mtei commented Feb 5, 2022

FYI:

Last September, I experimented with encoder_read() by rewriting it as follows.

As a result, the estimated execution time of encoder_read() in normal time went from 18 microseconds to 2 microseconds.

bool encoder_read(void) {
    bool changed = false;
    for (uint8_t i = 0; i < NUMBER_OF_ENCODERS; i++) {
        uint8_t new_status = (readPin(encoders_pad_a[i]) << 0) | (readPin(encoders_pad_b[i]) << 1);
        if ((encoder_state[i]&0x3) != new_status) {
            encoder_state[i] <<= 2;
            encoder_state[i] |= new_status;
            changed |= encoder_update(i, encoder_state[i]);
        }
    }
    return changed;
}

@tzarc
Copy link
Member Author

tzarc commented Feb 7, 2022

@mtei great, thanks, added to the PR.

@drashna
Copy link
Member

drashna commented Feb 7, 2022

FYI:

Last September, I experimented with encoder_read() by rewriting it as follows.

As a result, the estimated execution time of encoder_read() in normal time went from 18 microseconds to 2 microseconds.

...

Can confirm that it doesn't seam to break anything, at least.

Correction. There are some regressions here. It reintroduces the "trigger encoder on startup" issue, again.

@mtei
Copy link
Contributor

mtei commented Feb 13, 2022

Correction. There are some regressions here. It reintroduces the "trigger encoder on startup" issue, again.

You probably have to wait for the input signal to stabilize after the pull-up resistor is enabled.

diff --git a/quantum/encoder.c b/quantum/encoder.c
index 2f37bee6d9..83758a8cbd 100644
--- a/quantum/encoder.c
+++ b/quantum/encoder.c
@@ -121,7 +121,9 @@ void encoder_init(void) {
     for (uint8_t i = 0; i < thisCount; i++) {
         setPinInputHigh(encoders_pad_a[i]);
         setPinInputHigh(encoders_pad_b[i]);
-
+    }
+    wait_ms(1); // After the pull-up resistor is enabled, wait for the input signal to stabilize.
+    for (uint8_t i = 0; i < thisCount; i++) {
         encoder_state[i] = (readPin(encoders_pad_a[i]) << 0) | (readPin(encoders_pad_b[i]) << 1);
     }
 }

EDIT: I posted new PR #16372

@tzarc
Copy link
Member Author

tzarc commented Feb 16, 2022

@mtei great, will integrate #16372 into this PR once develop is merged and reopens.

@drashna
Copy link
Member

drashna commented Feb 17, 2022

Can confirm that fixes the issue I was seeing with this.

@tzarc tzarc force-pushed the fix-encoders-and-enable-tests branch 2 times, most recently from f36a583 to b5acb20 Compare March 5, 2022 23:24
@tzarc tzarc force-pushed the fix-encoders-and-enable-tests branch from b5acb20 to e9b9e71 Compare March 5, 2022 23:26
@tzarc tzarc merged commit 2f6751e into qmk:develop Mar 8, 2022
@tzarc tzarc deleted the fix-encoders-and-enable-tests branch March 8, 2022 05:58
waffle87 pushed a commit to waffle87/qmk_firmware that referenced this pull request Mar 23, 2022
@FWest98 FWest98 mentioned this pull request Jun 5, 2022
14 tasks
0xcharly pushed a commit to Bastardkb/bastardkb-qmk that referenced this pull request Jul 4, 2022
3araht added a commit to 3araht/qmk_firmware that referenced this pull request Aug 31, 2022
…Asymmetric encoders, encoder tests. (qmk#16068) is applied.)
3araht added a commit to 3araht/qmk_firmware that referenced this pull request Aug 31, 2022
…Asymmetric encoders, encoder tests. (qmk#16068) is applied.)
@3araht 3araht mentioned this pull request Aug 31, 2022
14 tasks
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.

3 participants