Rework encoders to enable asymmetric split keyboards #12090

BalzGuenat · 2021-03-02T21:35:09Z

Description

This allows asymmetric split keyboards to have a different number of encoders on each half.
Previously, the code assumed an equal number of encoders, even if they were wired differently.

This should be fully backwards compatible.

Types of Changes

Core
Enhancement/optimization
Documentation

Issues Fixed or Closed by This PR

[Bug] Using only right side encoder in split keyboard #10096

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

I will fix the code style while I wait for comments. I would appreciate some help with testing this, as I only know how to test this on hardware.

P.S.: Not sure if this counts as a core PR...

BalzGuenat · 2021-03-04T01:24:00Z

Heads up that these changes will almost certainly conflict with those in #11706. I'd be willing to resolve those conflicts when they arise.

BalzGuenat · 2021-03-04T21:56:11Z

So I've just spent a few hours unsuccessfully trying to get even a single unit test to compile. I've hit only brick walls. If anyone wants to help with that over here, I would be thankful.

That said, I've removed all efforts towards unit tests from this PR so that it's clean and ready to merge.

BalzGuenat · 2021-03-04T23:21:01Z

Two users/boards implement their own transport.c:

I'm leaving it to @drashna to adopt the changes I made to transport.c into those files.

BalzGuenat · 2021-03-06T02:23:12Z

I have finally managed to add some unit tests! Thanks a bunch to @ThreeFx for the help!

drashna · 2021-03-07T20:24:11Z

Two users/boards implement their own transport.c:

dactyl_manuform

drashna

I'm leaving it to @drashna to adopt the changes I made to transport.c into those files.

Those are all mine, so no worries. And I plan on deprecating them in the near future, as well.

I have finally managed to add some unit tests! Thanks a bunch to @ThreeFx for the help!

Huzzah!

drashna · 2021-03-07T20:28:57Z

docs/feature_encoders.md

+#define ENCODERS_PAD_A {  }
+#define ENCODERS_PAD_B {  }
+#define ENCODER_RESOLUTIONS {  }


It may be worth removing this.

In the encoder.c file, you could add:

#ifndef ENCODERS_PAD_A # define ENCODERS_PAD_A { } #endif

Or the like.

I was a bit worried about making things confusing. Leaving out the left side would have very different effect from leaving out the right side. Leave out the _RIGHT pads, the pads get mirrored and you get encoders on both sides. But leave out the "normal" (aka. left) pads and you only get encoders on right.

It would also mean that a board with encoders on only left would look different from a board with encoders only on right:

// only right #define ENCODERS_PAD_A_RIGHT { X } #define ENCODERS_PAD_A_RIGHT { Y } // only left #define ENCODERS_PAD_A { X } #define ENCODERS_PAD_A { Y } #define ENCODERS_PAD_A_RIGHT { } #define ENCODERS_PAD_A_RIGHT { }

Waddaya think?

Personally I'd prefer to explicitly require both sides in definitions, if an asymmetrical encoder setup is defined.
So you could do the #ifdef, but I'm not sure it's the most readable/understandable/maintainable.

So yeah, my vote would be both:

// only right #define ENCODERS_PAD_A { } #define ENCODERS_PAD_A { } #define ENCODERS_PAD_A_RIGHT { X } #define ENCODERS_PAD_A_RIGHT { Y } // only left #define ENCODERS_PAD_A { X } #define ENCODERS_PAD_A { Y } #define ENCODERS_PAD_A_RIGHT { } #define ENCODERS_PAD_A_RIGHT { }

BalzGuenat · 2021-04-08T12:09:24Z

It's been a while. Anything more I can do to get this merged?

stale · 2021-07-10T20:08:27Z

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 awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

stale · 2021-08-21T04:51:18Z

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.

# Conflicts: # build_test.mk # quantum/encoder.c # quantum/split_common/transport.c # testlist.mk

BalzGuenat · 2021-09-30T21:04:08Z

After months, I have updated my branch today. Could this please be reopened?

tzarc

Only real request for change is to make sure all the C/CPP/H files have GPL2+ copyright headers.

tzarc · 2021-11-01T17:10:44Z

docs/feature_encoders.md

+#define ENCODERS_PAD_A {  }
+#define ENCODERS_PAD_B {  }
+#define ENCODER_RESOLUTIONS {  }


Personally I'd prefer to explicitly require both sides in definitions, if an asymmetrical encoder setup is defined.
So you could do the #ifdef, but I'm not sure it's the most readable/understandable/maintainable.

So yeah, my vote would be both:

// only right #define ENCODERS_PAD_A { } #define ENCODERS_PAD_A { } #define ENCODERS_PAD_A_RIGHT { X } #define ENCODERS_PAD_A_RIGHT { Y } // only left #define ENCODERS_PAD_A { X } #define ENCODERS_PAD_A { Y } #define ENCODERS_PAD_A_RIGHT { } #define ENCODERS_PAD_A_RIGHT { }

drashna · 2021-11-19T18:43:05Z

have a couple of merge conflicts.

tzarc · 2021-11-19T21:36:54Z

Fixed the merge conflicts, however tests are currently failing. @BalzGuenat would you mind investigating with a make test:all?

tzarc · 2021-11-20T04:40:39Z

Merged with develop to ensure the unit test workflow executes on CI.

BalzGuenat · 2021-11-20T12:07:09Z

Fixed the tests. The callback override didn't work anymore since the signature changed in #12805.

drashna · 2021-11-20T18:17:37Z

Unfortunately, this looks like it broke a bunch of the encoder boards with:

quantum/split_common/transport.h:74:37: error: expected ‘)’ before ‘]’ token                                                                              
     uint8_t state[NUMBER_OF_ENCODERS];

And other errors.

This reverts commit 32215d5.

drashna · 2021-11-27T02:22:23Z

Just a heads up, this seems to have caused a bug during initialization.

Eg, it spams encoder updates until the board has normalized.

…)" This reverts commit 32215d5.

* Revert "fix broken macro in transport.h (#15239)" This reverts commit 06f18e2. * Revert "Rework encoders to enable asymmetric split keyboards (#12090)" This reverts commit 32215d5.

github-actions bot added core documentation labels Mar 2, 2021

drashna requested a review from a team March 3, 2021 03:17

Balz Guenat and others added 5 commits March 4, 2021 12:24

rework encoder pin handling for split keyboards

9b7be8b

simplify encoder code

c31e643

fixes and align resolutions

20f003f

update docs

6290092

fix formating of encoder.c with clang-format

91ae274

BalzGuenat force-pushed the rework-encoders branch from da514a0 to 91ae274 Compare March 4, 2021 20:26

replace include of quantum.h in encoder.h

8f2f14f

BalzGuenat force-pushed the rework-encoders branch from f3b7e8b to 8f2f14f Compare March 4, 2021 21:52

BalzGuenat marked this pull request as draft March 4, 2021 22:52

fix transport buffer size for encoders

f283d0f

BalzGuenat marked this pull request as ready for review March 4, 2021 23:13

add unit tests for encoders

b8f157b

drashna reviewed Mar 7, 2021

View reviewed changes

drashna requested a review from a team April 9, 2021 21:28

Merge branch 'develop' into rework-encoders

b8aba1b

stale bot added the awaiting changes label Jul 10, 2021

stale bot closed this Aug 21, 2021

Merge remote-tracking branch 'upstream/develop' into rework-encoders

bcd52bd

# Conflicts: # build_test.mk # quantum/encoder.c # quantum/split_common/transport.c # testlist.mk

tzarc added awaiting review breaking_change_2021q4 labels Oct 30, 2021

tzarc requested changes Nov 1, 2021

View reviewed changes

BalzGuenat added 2 commits November 3, 2021 17:32

add license header to encoder test files

15b1c51

Merge remote-tracking branch 'upstream/develop' into rework-encoders

ff6b611

BalzGuenat requested a review from tzarc November 3, 2021 16:35

tzarc approved these changes Nov 17, 2021

View reviewed changes

tzarc requested review from a team and drashna November 17, 2021 21:09

drashna approved these changes Nov 19, 2021

View reviewed changes

tzarc added 2 commits November 20, 2021 08:25

Merge branch 'develop' into rework-encoders

e41e946

Fix overall test execution, tests are failing.

d545e98

Merge remote-tracking branch 'upstream/develop' into rework-encoders

07d246d

fix encoder tests

1d9e349

drashna merged commit 32215d5 into qmk:develop Nov 20, 2021

drashna added a commit that referenced this pull request Nov 20, 2021

Revert "Rework encoders to enable asymmetric split keyboards (#12090)"

eeeb40f

This reverts commit 32215d5.

BalzGuenat mentioned this pull request Nov 20, 2021

Fix encoder bug from #12090 #15239

Merged

14 tasks

drashna added a commit to drashna/qmk_firmware that referenced this pull request Nov 27, 2021

Revert "Rework encoders to enable asymmetric split keyboards (qmk#12090…

96d7bde

…)" This reverts commit 32215d5.

zvecr mentioned this pull request Aug 28, 2022

[Bug] Using only right side encoder in split keyboard #10096

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework encoders to enable asymmetric split keyboards #12090

Rework encoders to enable asymmetric split keyboards #12090

BalzGuenat commented Mar 2, 2021 •

edited

Loading

BalzGuenat commented Mar 4, 2021

BalzGuenat commented Mar 4, 2021

BalzGuenat commented Mar 4, 2021 •

edited

Loading

BalzGuenat commented Mar 6, 2021

drashna commented Mar 7, 2021

drashna Mar 7, 2021

BalzGuenat Mar 8, 2021

tzarc Nov 1, 2021

BalzGuenat commented Apr 8, 2021

stale bot commented Jul 10, 2021

stale bot commented Aug 21, 2021

BalzGuenat commented Sep 30, 2021

tzarc left a comment

tzarc Nov 1, 2021

drashna commented Nov 19, 2021

tzarc commented Nov 19, 2021

tzarc commented Nov 20, 2021

BalzGuenat commented Nov 20, 2021

drashna commented Nov 20, 2021

drashna commented Nov 27, 2021

Rework encoders to enable asymmetric split keyboards #12090

Rework encoders to enable asymmetric split keyboards #12090

Conversation

BalzGuenat commented Mar 2, 2021 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

BalzGuenat commented Mar 4, 2021

BalzGuenat commented Mar 4, 2021

BalzGuenat commented Mar 4, 2021 • edited Loading

BalzGuenat commented Mar 6, 2021

drashna commented Mar 7, 2021

drashna Mar 7, 2021

Choose a reason for hiding this comment

BalzGuenat Mar 8, 2021

Choose a reason for hiding this comment

tzarc Nov 1, 2021

Choose a reason for hiding this comment

BalzGuenat commented Apr 8, 2021

stale bot commented Jul 10, 2021

stale bot commented Aug 21, 2021

BalzGuenat commented Sep 30, 2021

tzarc left a comment

Choose a reason for hiding this comment

tzarc Nov 1, 2021

Choose a reason for hiding this comment

drashna commented Nov 19, 2021

tzarc commented Nov 19, 2021

tzarc commented Nov 20, 2021

BalzGuenat commented Nov 20, 2021

drashna commented Nov 20, 2021

drashna commented Nov 27, 2021

BalzGuenat commented Mar 2, 2021 •

edited

Loading

BalzGuenat commented Mar 4, 2021 •

edited

Loading