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

Add new function for encode module #16954

Closed
wants to merge 5 commits into from

Conversation

KeychronMacro
Copy link
Contributor

@KeychronMacro KeychronMacro commented Apr 28, 2022

Improved encoder

Description

  • Improve the encoder and make it perform better, specifcally add a function called "encoder_insert_state", and will be called in pal set line event callback function when we turn the knob every time.

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: 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).

Fixed encoder and update q2
@github-actions github-actions bot added core keyboard keymap via Adds via keymap and/or updates keyboard for via support labels Apr 28, 2022
@zvecr
Copy link
Member

zvecr commented Apr 28, 2022

Core changes should be separate to keyboard changes.

@lokher
Copy link
Contributor

lokher commented Apr 29, 2022

Core changes should be separate to keyboard changes.

We added a new function to encoder.h and encoder.c, which is only invoked by Q2, and it won't break anything, why do we need to separate it? It's kinda wierd if we just add a single function, maybe someone would ask how to use it to fix this issue.

@zvecr
Copy link
Member

zvecr commented Apr 29, 2022

Because that is the process we ask people to follow, as it allows us manage the project easier.

By default core changes also have to go via develop, so i have updated the merge target (this is within the PR checklist).

@zvecr zvecr changed the base branch from master to develop April 29, 2022 01:20
This was referenced Apr 30, 2022
@FWest98
Copy link

FWest98 commented Jun 5, 2022

This change no longer works properly - it causes the rotary encoder to stop functioning altogether. This is due to the new if-condition at https://github.com/qmk/qmk_firmware/blob/master/quantum/encoder.c#L197, introduced in #16068. It prevents the encoder from triggering an action when this code path does not detect a state change - this will almost always be the case since encoder_insert_state already updates this state. The best solution I found to this issue is to introduce an additional condition that will trigger the update function to be called after the external update. See: FWest98@95fbce9#diff-c8e0671af976b0c896b651211e9605562985027873e5ef0d8a51a74c17aa8a6e

@github-actions
Copy link

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Aug 29, 2022
@drashna drashna requested a review from a team September 1, 2022 03:17
@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Sep 2, 2022
@github-actions
Copy link

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Oct 18, 2022
@spacecakes
Copy link

Any updates on this? Is it still relevant? @lokher

@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Nov 17, 2022
quantum/encoder.c Show resolved Hide resolved
@@ -20,7 +20,8 @@ ENCODER_ENABLE = yes # Enable Encoder
DIP_SWITCH_ENABLE = yes
RGB_MATRIX_ENABLE = yes
RGB_MATRIX_DRIVER = CKLED2001
EEPROM_DRIVER = i2c
# EEPROM_DRIVER = i2c
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

This would remove any eeprom support from the board and make via support useless.

@drashna drashna requested a review from a team November 20, 2022 05:35
@github-actions github-actions bot removed via Adds via keymap and/or updates keyboard for via support keyboard keymap labels Nov 21, 2022
@KeychronMacro KeychronMacro changed the title Fix Keychron Q2 rotary encoder detection issue Add new function for encode update Nov 21, 2022
@KeychronMacro KeychronMacro changed the title Add new function for encode update Add new function for encode module Nov 21, 2022
@github-actions
Copy link

github-actions bot commented Jan 6, 2023

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Jan 6, 2023
@@ -40,6 +40,7 @@ extern volatile bool isLeftHand;

static pin_t encoders_pad_a[NUM_ENCODERS_MAX_PER_SIDE] = ENCODERS_PAD_A;
static pin_t encoders_pad_b[NUM_ENCODERS_MAX_PER_SIDE] = ENCODERS_PAD_B;
static bool encoder_external_update[NUM_ENCODERS] = {false};
Copy link
Member

Choose a reason for hiding this comment

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

This is set-only and never consulted.
As far as I can tell the issue raised by @FWest98 has not been addressed as a result, suggest revisiting this before it goes stale again.

@tzarc tzarc removed the stale Issues or pull requests that have become inactive without resolution. label Jan 6, 2023
@github-actions
Copy link

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Feb 21, 2023
@tzarc
Copy link
Member

tzarc commented Feb 21, 2023

We'll be moving towards a decoupled model with ENCODER_DRIVER = xxxx instead of continuing here.

@tzarc tzarc closed this Feb 21, 2023
@lokher lokher deleted the fix_encoder branch August 12, 2024 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core stale Issues or pull requests that have become inactive without resolution.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants