Enables use of custom keycodes on Torn encoders #12887

ky1ejs · 2021-05-13T11:53:24Z

Description

I noticed that custom key codes were not working via the encoders on my Torn kb.

It seems that the keymap.c isn't called to check if it wants to override a given keycode on encoder rotation. This PR attempts to fix that.

I've notified the keyboard designer (@rtitmuss) that I will be trying to make this fix.

Types of Changes

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

ky1ejs · 2021-05-13T11:54:36Z

keyboards/torn/torn_encoder.c

@@ -32,6 +32,7 @@ static uint8_t encoder_state  = 0;
 static int8_t  encoder_pulses = 0;

 __attribute__((weak)) extern const uint16_t PROGMEM encoder_keymaps[][2][2];
+bool encoder_update_user(uint16_t keycode, bool clockwise);


I have little-to-no experience with C, so I'm not sure if this is the right way to define this function that can be overridden by the keymap.

Is there a better way to do this?

That won't work right now, because it's a different type for the function. However, I do have a PR that changes the encoder code to support this. #12805 specifically.

For now...

Suggested change

bool encoder_update_user(uint16_t keycode, bool clockwise);

__attribute__((weak)) encoder_update_user(uint16_t keycode, bool clockwise);

It works for me, but I'm sure you mean some other kind of "works". Can I use is with the bool return type for now? I need to know if the keymap overrode the given key code so that torn_encoder.c knows to not send a keycode itself.

It looks like your merged PR does this? Is there a proper way I can fall in line with those changes that you made?

if you target develop instead, yes. Otherwise, it's wait for the breaking changes process, and update this in june.

Also, you may be interested in:
#12982

Oh nice! Missed that this repo is using gitflow.

#12982 is confusing me a bit. How would it look like to set the keycodes triggered on encoder rotation in the LAYOUT? And when you do, are custom keycodes that are set for encoder rotations in this way handled in the same ..._kb macro function for the keymap level?

I'll look at rebasing onto develop and updating this PR, thank you!

@drashna done, how does it look now?

I noticed that encoder_update_user() passes an index develop, but with the keymap is passes a keycode to the user's keymap. Am I using it right?

the PR in question changes how the encoder code works. It points to a location in the switch matrix, and then calls that via "action_exec", which is the lowest level event for a switch being triggered. This allows non-basic keycodes (including macros) to be used for the encoder, which also adds VIA support for encoders, since the keycode for them is set in the keymap.

Which is why I mentioned it. And it is most likely the way we're headed rather than the encoder_keymap type method.

As for the callback, I mistyped above. The main difference between master and develop is that the callbacks are now boolean, which allows for them to be over-ridden, if/when needed. I'm just ... very used to typing out the process_record_* function...

eg:

Suggested change

bool encoder_update_user(uint16_t keycode, bool clockwise);

bool encoder_update_user(uint8_t index, bool clockwise);

Yeh just noticed that it's a uint8_t instead of a uint16_t, so it can't take custom passed key bodes like the keyboard switch press custom function can:

bool process_record_user(uint16_t keycode, keyrecord_t *record) {

Would be nice if the new encoder function that returns a bool could have the first argument be the key code, rather than the index so the keymap doesn't have to lookup the code.

@drashna whoops! I hadn't realised you'd replied for sending my last comment.

So does that mean that the keycodes that encores send on rotate also need to be assigned to a switch?

For now I've crudely made a torn version of the method until I get this working with the new approach you've made @drashna.

I'm still not sure how clear it is for encoder rotation keycodes to be in the switch layout. I think it makes it harder to read the layout, rather than separating them.

stale · 2021-07-08T00:46:49Z

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.

drashna · 2021-07-20T16:45:26Z

keyboards/torn/torn_encoder.c

@@ -52,7 +50,9 @@ bool encoder_update_kb(uint8_t index, bool clockwise) {
      code = encoder_default[index][clockwise];
    }

-    tap_code16(code);
+    if (encoder_update_user(code, clockwise) == false) {


Logic is inverted. False is that the kb code should NOT be respected.

Suggested change

if (encoder_update_user(code, clockwise) == false) {

if (encoder_update_user(code, clockwise)) {

stale · 2021-09-04T23:36:05Z

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-10-11T05:46:21Z

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.

github-actions bot added the keyboard label May 13, 2021

ky1ejs commented May 13, 2021

View reviewed changes

ky1ejs mentioned this pull request May 13, 2021

Adding kylejm's keymaps #12167

Closed

14 tasks

Enables use of custom keycodes on Torn encoders

6383d63

ky1ejs force-pushed the fix-custom-keycodes-on-torn-encoders branch from f068e69 to 6383d63 Compare May 23, 2021 10:41

ky1ejs changed the base branch from master to develop May 23, 2021 10:41

stale bot added the awaiting changes label Jul 8, 2021

drashna requested changes Jul 20, 2021

View reviewed changes

stale bot removed the awaiting changes label Jul 20, 2021

stale bot added the awaiting changes label Sep 4, 2021

stale bot closed this Oct 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enables use of custom keycodes on Torn encoders #12887

Enables use of custom keycodes on Torn encoders #12887

ky1ejs commented May 13, 2021

ky1ejs May 13, 2021

drashna May 22, 2021

ky1ejs May 22, 2021

drashna May 22, 2021 •

edited

ky1ejs May 23, 2021 •

edited

ky1ejs May 23, 2021

drashna May 23, 2021

ky1ejs May 23, 2021 •

edited

ky1ejs May 23, 2021

ky1ejs May 23, 2021

stale bot commented Jul 8, 2021

drashna Jul 20, 2021

stale bot commented Sep 4, 2021

stale bot commented Oct 11, 2021

	bool encoder_update_user(uint16_t keycode, bool clockwise);
	__attribute__((weak)) encoder_update_user(uint16_t keycode, bool clockwise);

	bool encoder_update_user(uint16_t keycode, bool clockwise);
	bool encoder_update_user(uint8_t index, bool clockwise);

	if (encoder_update_user(code, clockwise) == false) {
	if (encoder_update_user(code, clockwise)) {

Enables use of custom keycodes on Torn encoders #12887

Enables use of custom keycodes on Torn encoders #12887

Conversation

ky1ejs commented May 13, 2021

Description

Types of Changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drashna May 22, 2021 • edited

Choose a reason for hiding this comment

ky1ejs May 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ky1ejs May 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jul 8, 2021

Choose a reason for hiding this comment

stale bot commented Sep 4, 2021

stale bot commented Oct 11, 2021

drashna May 22, 2021 •

edited

ky1ejs May 23, 2021 •

edited

ky1ejs May 23, 2021 •

edited