add split&matrix encoder support

[hsakoh]

Enables flexible wiring of Encoders.

Description

The following issues are in the current encoder feature.

• The same number of encoders are needed for left and right keyboards ([Bug] Using only right side encoder in split keyboard #10096).
• The A/B line of Encoder occupies the pin.
  　(It is not impossible to wire A/B line to the matrix, but if you wire to GND, you cannot wire in parallel.)

This solves the two issues above.
Similar to #7209, but #10096 will also be improved.

I created and tested the circuit diagram on this gist on a breadboard.

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

• [Bug] Using only right side encoder in split keyboard #10096
• Adds Encoder Matrix Functionality #7209

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've only been touching QMK for a week now, I may have a fundamental misunderstanding.
So please be nice and gentle.

[hsakoh]

The changes in #10259 have also been incorporated into this PullRequest feature

[hsakoh]

I'm adding a large ifdef and indenting it.
So when reviewing it, I think it would be easier to see if you add "?w=1" like this.
https://github.com/qmk/qmk_firmware/pull/10517/files?w=1

[Erovia]

__attribute__((weak)) ✔️
I don't have the HW to test this.

This is just nitpicking, but IMHO simply "Basic/Advanced" would be better terms than "Basic/Flexible Support".

[hsakoh]

In JP's Discord, mtei pointed out that "peek_matrix can improve the scan rate",
so I implemented it.

[mtei]

The "#if defined()" in the source makes it hard for me to read, so my first comment will focus on mitigating the readability with a formal change.

The following pattern appears four times. The conditional expression is complex.

#if defined(SPLIT_KEYBOARD) && defined(MATRIX_ENCODER_PINS_ABC) && defined(MATRIX_ENCODER_PINS_ABC_RIGHT)
//split keyboard,both encoder
   :
   :
#elif defined(SPLIT_KEYBOARD) && !defined(MATRIX_ENCODER_PINS_ABC) && defined(MATRIX_ENCODER_PINS_ABC_RIGHT)
//split keyboard,right only encoder
   :
   :
#else
//non split keyboard or left only encoder
   :
   :
#endif

I made it a little easier to read. -->
mtei@1f97d33#diff-370757d2df51ae456bf63c165fc71817
in https://github.com/mtei/qmk_firmware/tree/10517_matrix_encoder-patch-1

[mtei]

The following three functions are easier to read if you separate the body part of the process as static inline functions.

• void encoder_update()
• void encoder_read_all()
• void encoder_init()

Here is an example of encoder_read_all().

static inline 
void encoder_read_all_common(pin_t *current_rows, pin_t *current_matrix, 
                             uint8_t encoder_count,  encoder_t *current_encoders)
{
    bool       isFirstScan      = true;
    for (int i = 0; i < encoder_count; i++) {
        ////
    }
}

static void encoder_read_all(void) {
#    if defined(split_keyboard_both_encoder)
    if (isLeftHand) {
        encoder_read_all_common(row_pins_left, 
                                (pin_t *)matrix_encoders_pins_left,
                                NUMBER_OF_ENCODERS_LEFT,
                                encoders_left);
    } else {
        encoder_read_all_common(row_pins_right,
                                (pin_t *)matrix_encoders_pins_right,
                                NUMBER_OF_ENCODERS_RIGHT, 
                                encoders_right);
    }
#    elif defined(split_keyboard_right_only_encoder)
    encoder_read_all_common(row_pins_right,
                            (pin_t *)matrix_encoders_pins_right, 
                            NUMBER_OF_ENCODERS_RIGHT,
                             encoders_right);
#    else  // non split keyboard or left only encoder
    encoder_read_all_common(row_pins,
                            (pin_t *)matrix_encoders_pins,
                            NUMBER_OF_ENCODERS,
                            encoders)
#    endif
}

[hsakoh]

The code has been modified in response to the following review.
#10517 (comment)
->a1d6f54

#10517 (comment)
->9a1e16b

[russbot]

I just wanted to leave a comment, because I was playing with this PR.
It seems like encoders without dents, or "jog" / light torque ones can get stuck, which interrupts all of the encoders.
I haven't run into any issues with encoders with pronounced dents. I was testing with Alps EC12E24404A8.

#define MATRIX_ENCODER_PINS_ABC {{B1,B2,B6},{B1,B2,F6}}
B1 and B2 were wired directly to the mcu
B6 and F6 were wired to a matrix row with a diode

edit: Never mind, I was able to get my other encoders (with pronounced dents) to lock up by moving 2 at a time pretty fast and then turning one and then the other. Not sure if this would happen in normal usage.

[hsakoh]

@russbot
Thanks for the feedback.

I was able to reproduce the behavior of the comments using PEC12R.
By stopping the rotation in the middle, it can be reproduced by encoder with a detent.

The A-B phase of the rotary encoder takes four states: (0,0)(1,0)(1,1)(0,1).
This diagram represents the internal circuit of the encoder.

With Type A wiring, when one of them is at (1,1), it interferes with the other one's reading.
I don't think this behavior would occur with Type B wiring.
Can you verify this with TypeB wiring?

If the Type B wiring seems to solve the problem, I'll add it to the docs.
I think this problem is not as frequent with encoders with detents.

[russbot]

@hsakoh awesome thanks, i haven't been able to get it to stick once with, type b, on any of my encoders.

[mtei]

I'm not sure why a Type-B circuit needs a diode to connect to C.

[hsakoh]

Haha, so yeah sure, Type B doesn't need a diode in C.

[hsakoh]

Conflict is happening, so I'm going to rebase

[hsakoh]

I don't want to have this Pull Request for a long time because conflicts happen occasionally,
so someone please complete the review and merge.

@Erovia
Or could you assign another reviewer?

[mtei]

I tested by connecting encoders to each side of the split keyboard (Helix keyboard) one by one.

• test 1

  // config.h
  #define ENCODERS_PAD_A { B6 }
  #define ENCODERS_PAD_B { B5 }

  • master (6b1ae7e)
    ok
  • PR 10517 (6ed8fbb), PR 10517 (b6cac82)
    left(master) ok
    right(slave) encoder don't work
  • PR 10517 (ac7721f00)
    ok
• test 2

  // config.h
  #define ENCODERS_PAD_A { B6 }
  #define ENCODERS_PAD_B { B5 }
  #define ENCODERS_PAD_A_RIGHT { B6 }
  #define ENCODERS_PAD_B_RIGHT { B5 }

  • master (6b1ae7e)
    ok
  • PR 10517 (6ed8fbb), PR 10517 (b6cac82)
    left(master) ok
    right(slave) encoder don't work
  • PR 10517 (ac7721f00)
    ok
• test 3

  /* Encorder */
  #define ENCODERS_PAD_A_RIGHT { B6 }
  #define ENCODERS_PAD_B_RIGHT { B5 }

  • can't build. This is as expected.
• test 3 b

  /* Encorder */
  #define ENCODERS_PAD_A { NO_PIN }
  #define ENCODERS_PAD_B { NO_PIN }
  #define ENCODERS_PAD_A_RIGHT { B6 }
  #define ENCODERS_PAD_B_RIGHT { B5 }

  • master (6b1ae7e)
    left(master) NO_PIN ok
    right(slave) ok (but index is 2 and 3) (but index is 1 not 0. This is as expected)
  • PR 10517 (6ed8fbb), PR 10517 (b6cac82)
    encoder don't work
  • PR 10517 (ac7721f00)
    left(master) don't work OK
    right(slave) index is 1 not 0. This is as expected. ok
• test 4

  // config.h
  #define MATRIX_ENCODER_PINS_ABC {{B6,B5,NO_PIN}}
  //  test1
  //  #define ENCODERS_PAD_A { B6 }
  //  #define ENCODERS_PAD_B { B5 }

  • PR 10517 (6ed8fbb), PR 10517 (b6cac82), PR 10517 (ac7721f00), PR 10517 (b45532aa8)
    left(master) ok
    right(slave) encoder don't work. This is as expected.
• test 5

  // config.h
  #define MATRIX_ENCODER_PINS_ABC {{B6,B5,NO_PIN}}
  #define MATRIX_ENCODER_PINS_ABC_RIGHT {{B6,B5,NO_PIN}}

  • PR 10517 (6ed8fbb)
    Can't build.
  • PR 10517 (b6cac82)
    left(master) ok
    right(slave) encoder don't work
  • PR 10517 (ac7721f00), PR 10517 (b45532aa8)
    ok
• test 6

  // config.h
  #define MATRIX_ENCODER_PINS_ABC_RIGHT {{B6,B5,NO_PIN}}

  • PR 10517 (ac7721f00)
    left(master) work ! This is unexpected. index is 0. Why ?
    right(slave) index is 0. This is as expected. ok
  • PR 10517 (b45532aa8)
    OK
    --
    The tests above is the case where the 'C' pin is connected to GND.

[hsakoh]

@mtei
I can't confirm the entire config, so I can't say for sure.
Test 1 should work.
Test 2 should work.
Test 3 is as expected.
Test 3b What does it mean that the Index will be 2 and 3? There's only one encoder, right?
Test 4 is as expected.
Test 5 Show me what kind of build errors you're getting

I don't have a Helix keyboard.
Show me the entire firmware config specifically.
It's easier to check if there is a branch for each test.

[hsakoh]

To restart TravisCI, close and open ,
it because I don't have write permission.

[hsakoh]

The review doesn't go forward, so I no longer have the motivation to contribute.
I'll close the pull request and delete the entire repository.
I won't contribute, so don't violate my moral rights regarding this change.

Goodbye qmk.
Good product, but the collaborators are not good.

[russbot]

I was really hoping this would make it in

[drashna]

Were sorry to see this.

For travis CI, we're running into issues with it timing out and errorring out. As long as affected boards aren't erroring from compiling, that's fine.

As for getting it merged, that cannot and will not get merged because you don't have time. We do this in our free time, as well. We don't get paid for this, so it can be hard to get free time to review. Also we have different expertise and different areas of the codebase that they're familiar with.

We are more than willing to work with you, but the process will take time, so it follows the existing coding convention and doesn't break stuff.

We would still like to work with you on this, if you're willing.