Fix for encoders and support ENCODER_MAP_ENABLE on Planck rev7 #23967

ykeisuke · 2024-06-20T17:15:51Z

Description

Fix for encoders on Planck rev7
Support ENCODER_MAP_ENABLE feature on Planck rev7
Update readme.md on Planck rev7

Types of Changes

Issues Fixed or Closed by This PR

None

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

keyboards/planck/rev7/keymaps/default/keymap.c

keyboards/planck/rev7/matrix.c

tzarc · 2024-06-24T21:24:16Z

keyboards/planck/rev7/matrix.c

+static void encoder_handle_state_change(uint8_t index, uint8_t state) {
+    uint8_t i = index;
+
+    const uint8_t resolution = ENCODER_RESOLUTION;
+
+    encoder_pulses[i] += encoder_LUT[state & 0xF];
+
+    if (encoder_pulses[i] >= resolution) {
+        encoder_queue_event(index, ENCODER_COUNTER_CLOCKWISE);
+    }
+
+    // direction is arbitrary here, but this clockwise
+    if (encoder_pulses[i] <= -resolution) {
+        encoder_queue_event(index, ENCODER_CLOCKWISE);
+    }
+    encoder_pulses[i] %= resolution;
+
+}
+
+void encoder_driver_task(void) {
+
+    // set up C/rows for encoder read
+    for (int i = 0; i < MATRIX_ROWS; i++) {
+        gpio_set_pin_output(matrix_row_pins[i]);
+        gpio_write_pin_high(matrix_row_pins[i]);
+    }
+
+    // set up A & B for reading
+    gpio_set_pin_input_high(B12);
+    gpio_set_pin_input_high(B13);
+
+    for (int i = 0; i < MATRIX_ROWS; i++) {
+        gpio_write_pin_low(matrix_row_pins[i]);
+        wait_us(PLANCK_ENCODER_SETTLE_PIN_STATE_DELAY);
+        uint8_t new_status = (palReadPad(GPIOB, 12) << 0) | (palReadPad(GPIOB, 13) << 1);
+        if ((encoder_state[i] & 0x3) != new_status) {
+            encoder_state[i] <<= 2;
+            encoder_state[i] |= new_status;
+            encoder_handle_state_change(i, encoder_state[i]);
+        }
+        gpio_write_pin_high(matrix_row_pins[i]);
+    }
+
+    // revert A & B to matrix state
+    gpio_set_pin_input_low(B12);
+    gpio_set_pin_input_low(B13);
+
+    // revert C/rows to matrix state
+    for (int i = 0; i < MATRIX_ROWS; i++) {
+        gpio_set_pin_input_low(matrix_row_pins[i]);
+    }
+
 }


This rolls back encoder abstraction PR, so I can't approve it.
Work out what the actual issue is, rather than blindly reverting the code.

@tracyde Thank you for your review. I see. Of course, If can use abstraction codes, I think that needs use it.
~~but does not work with abstraction code.~~
~~but I tried update code with encoder_quadrature_handle_read. what do you think?~~
~~1cf0cc6~~

Note: This message is old. @sigprof tells me good(?) code after commit code. Thank you.

I did update code. Please continue check. e34700f

sigprof · 2024-06-25T07:01:21Z

keyboards/planck/rev7/matrix.c

+#if defined(ENCODER_ENABLE) || defined(ENCODER_MAP_ENABLE)
+
+#if !defined(PLANCK_ENCODER_SETTLE_PIN_STATE_DELAY)
+#   define PLANCK_ENCODER_SETTLE_PIN_STATE_DELAY 20
+#endif
+
+void encoder_driver_task(void) {
+
+    // set up C/rows for encoder read
+    for (int i = 0; i < MATRIX_ROWS; i++) {
+        gpio_set_pin_output(matrix_row_pins[i]);
+        gpio_write_pin_high(matrix_row_pins[i]);
+    }
+
+    // set up A & B for reading
+    gpio_set_pin_input_high(B12);
+    gpio_set_pin_input_high(B13);
+
+    for (int i = 0; i < MATRIX_ROWS; i++) {
+        gpio_write_pin_low(matrix_row_pins[i]);
+        wait_us(PLANCK_ENCODER_SETTLE_PIN_STATE_DELAY);
+        encoder_quadrature_handle_read(i, palReadPad(GPIOB, 12), palReadPad(GPIOB, 13));
+        gpio_write_pin_high(matrix_row_pins[i]);
+    }
+
+    // revert A & B to matrix state
+    gpio_set_pin_input_low(B12);
+    gpio_set_pin_input_low(B13);
+
+    // revert C/rows to matrix state
+    for (int i = 0; i < MATRIX_ROWS; i++) {
+        gpio_set_pin_input_low(matrix_row_pins[i]);
+    }
+
 }
+
+#endif // ENCODER_ENABLE || ENCODER_MAP_ENABLE


Given the custom matrix implementation, I guess that something like this should work:

Suggested change

#if defined(ENCODER_ENABLE) || defined(ENCODER_MAP_ENABLE)

#if !defined(PLANCK_ENCODER_SETTLE_PIN_STATE_DELAY)

# define PLANCK_ENCODER_SETTLE_PIN_STATE_DELAY 20

#endif

void encoder_driver_task(void) {

// set up C/rows for encoder read

for (int i = 0; i < MATRIX_ROWS; i++) {

gpio_set_pin_output(matrix_row_pins[i]);

gpio_write_pin_high(matrix_row_pins[i]);

}

// set up A & B for reading

gpio_set_pin_input_high(B12);

gpio_set_pin_input_high(B13);

for (int i = 0; i < MATRIX_ROWS; i++) {

gpio_write_pin_low(matrix_row_pins[i]);

wait_us(PLANCK_ENCODER_SETTLE_PIN_STATE_DELAY);

encoder_quadrature_handle_read(i, palReadPad(GPIOB, 12), palReadPad(GPIOB, 13));

gpio_write_pin_high(matrix_row_pins[i]);

}

// revert A & B to matrix state

gpio_set_pin_input_low(B12);

gpio_set_pin_input_low(B13);

// revert C/rows to matrix state

for (int i = 0; i < MATRIX_ROWS; i++) {

gpio_set_pin_input_low(matrix_row_pins[i]);

}

}

#endif // ENCODER_ENABLE || ENCODER_MAP_ENABLE

#if !defined(PLANCK_ENCODER_SETTLE_PIN_STATE_DELAY)

# define PLANCK_ENCODER_SETTLE_PIN_STATE_DELAY 20

#endif

void encoder_quadrature_init_pin(uint8_t index, bool pad_b) {

// encoder_init() is called before matrix_init(), so the pin init needs to be done here.

pin_t col_pin = pad_b ? B13 : B12;

gpio_set_pin_input_low(col_pin);

gpio_set_pin_input_low(matrix_row_pins[index]);

}

uint8_t encoder_quadrature_read_pin(uint8_t index, bool pad_b) {

pin_t col_pin = pad_b ? B13 : B12;

gpio_set_pin_high(col_pin);

wait_us(PLANCK_ENCODER_SETTLE_PIN_STATE_DELAY);

uint8_t ret = gpio_read_pin(matrix_row_pins[index]) ? 0 : 1;

gpio_set_pin_input_low(col_pin);

return ret;

}

(Although the matrix code uses gpio_set_pin_low() instead of gpio_set_pin_input_low() for inactive columns; maybe it should be used here too, then the encoder pin init code in matrix_init_custom() needs to be changed to match.)

It could be possible to avoid the multiple settle delays by reading the state of all encoders at once, but with the current API it would require either making some assumptions about the order of encoder_quadrature_read_pin() calls (and in the existing code the order of calls for pads A and B is actually unspecified), or overriding encoder_driver_task().

And this implementation of encoder_quadrature_init_pin() might still not be enough, because it does not init the other column pins, so they might interfere with the encoder_quadrature_read_pin() calls done during encoder_quadrature_post_init() (that's unlikely though, because it would require that some keys are held down during startup). Doing things in a 100% correct way may require moving the matrix pin init into keyboard_pre_init_kb(), then encoder_quadrature_init_pin() may be empty (the default implementation still needs to be overridden though).

@sigprof Thank you for your review and sample code.

I may do not understand your sample code, I tried use your code on my local env and but does not work correct. so I will try find other way as reference to your sample code, and I need a time more.

Note: does not work code.

Suggested change

#if defined(ENCODER_ENABLE) || defined(ENCODER_MAP_ENABLE)

#if !defined(PLANCK_ENCODER_SETTLE_PIN_STATE_DELAY)

# define PLANCK_ENCODER_SETTLE_PIN_STATE_DELAY 20

#endif

void encoder_driver_task(void) {

// set up C/rows for encoder read

for (int i = 0; i < MATRIX_ROWS; i++) {

gpio_set_pin_output(matrix_row_pins[i]);

gpio_write_pin_high(matrix_row_pins[i]);

}

// set up A & B for reading

gpio_set_pin_input_high(B12);

gpio_set_pin_input_high(B13);

for (int i = 0; i < MATRIX_ROWS; i++) {

gpio_write_pin_low(matrix_row_pins[i]);

wait_us(PLANCK_ENCODER_SETTLE_PIN_STATE_DELAY);

encoder_quadrature_handle_read(i, palReadPad(GPIOB, 12), palReadPad(GPIOB, 13));

gpio_write_pin_high(matrix_row_pins[i]);

}

// revert A & B to matrix state

gpio_set_pin_input_low(B12);

gpio_set_pin_input_low(B13);

// revert C/rows to matrix state

for (int i = 0; i < MATRIX_ROWS; i++) {

gpio_set_pin_input_low(matrix_row_pins[i]);

}

}

#endif // ENCODER_ENABLE || ENCODER_MAP_ENABLE

#if defined(ENCODER_ENABLE) || defined(ENCODER_MAP_ENABLE)

void encoder_quadrature_init_pin(uint8_t index, bool pad_b) {

// encoder_init() is called before matrix_init(), so the pin init needs to be done here.

pin_t col_pin = pad_b ? B13 : B12;

gpio_set_pin_input_low(col_pin);

gpio_set_pin_input_low(matrix_row_pins[index]);

}

uint8_t encoder_quadrature_read_pin(uint8_t index, bool pad_b) {

pin_t col_pin = pad_b ? B13 : B12;

gpio_set_pin_input_high(col_pin); // If using `gpio_set_pin_high`, compile error. so i did change to this.

wait_us(PLANCK_ENCODER_SETTLE_PIN_STATE_DELAY);

uint8_t ret = gpio_read_pin(matrix_row_pins[index]) ? 0 : 1;

gpio_set_pin_input_low(col_pin);

return ret;

}

#endif // ENCODER_ENABLE || ENCODER_MAP_ENABLE

Oops, that was a typo, gpio_set_pin_high should have been gpio_write_pin_high; replacing it with gpio_set_pin_input_high would break things.

(And gpio_set_pin_output(col_pin) is needed before.)

And your code which switches the row pins to the output mode somewhat defeats the point of the custom matrix implementation (to avoid using the C13…C15 pins as outputs). My suggestion tries to keep those pins in the input mode, as the custom matrix code does (but apparently writing the code in the GitHub comment box is too hard).

@sigprof I did update code with blank encoder_quadrature_init_pin function. (already called gpio_set_pin_input_low for B12 and B13 on matrix_init_custom(void))

My suggestion tries to keep those pins in the input mode, as the custom matrix code does

I'm not confident. but updated. Is this commit expected code? e34700f

encoder_init() is called before matrix_init(), so the pin init needs to be done here.

That commit has blank encoder_quadrature_init_pin function. but looks no problem for me on using. Should that use encoder_quadrature_init_pin? and needs delete init code in matrix_init_custom?

* Fix for encoders * Support ENCODER_MAP_ENABLE feature * Update readme.md

…AP_ENABLE, play melody.

ykeisuke · 2024-06-29T06:57:53Z

@drashna @sigprof @tzarc Excuse me, Could you continue review?

ykeisuke changed the base branch from master to develop June 20, 2024 17:16

github-actions bot added keyboard keymap labels Jun 20, 2024

drashna requested changes Jun 20, 2024

View reviewed changes

keyboards/planck/rev7/keymaps/default/keymap.c Outdated Show resolved Hide resolved

keyboards/planck/rev7/matrix.c Outdated Show resolved Hide resolved

keyboards/planck/rev7/matrix.c Outdated Show resolved Hide resolved

keyboards/planck/rev7/matrix.c Outdated Show resolved Hide resolved

drashna requested a review from a team June 20, 2024 18:00

ykeisuke force-pushed the fix/planck-v7-with-encoder branch from 280c1d4 to 78ffe75 Compare June 21, 2024 07:37

ykeisuke requested a review from drashna June 23, 2024 03:58

ykeisuke force-pushed the fix/planck-v7-with-encoder branch 3 times, most recently from c81492d to f52be72 Compare June 24, 2024 13:47

tzarc reviewed Jun 24, 2024

View reviewed changes

sigprof reviewed Jun 25, 2024

View reviewed changes

github-actions bot added the core label Jun 25, 2024

ykeisuke force-pushed the fix/planck-v7-with-encoder branch from 1cf0cc6 to e34700f Compare June 25, 2024 14:42

github-actions bot removed the core label Jun 25, 2024

ykeisuke requested review from tzarc and sigprof June 26, 2024 01:55

ykeisuke force-pushed the fix/planck-v7-with-encoder branch 2 times, most recently from 180a2fc to 634f9c4 Compare June 27, 2024 06:58

ykeisuke added 3 commits June 29, 2024 15:54

Fix and Support ENCODER_MAP_ENABLE on Planck rev7

034f68a

* Fix for encoders * Support ENCODER_MAP_ENABLE feature * Update readme.md

Replace no need functions and usage text, and even if using ENCODER_M…

5947cb7

…AP_ENABLE, play melody.

Use abstract codes

45167b6

ykeisuke force-pushed the fix/planck-v7-with-encoder branch from 634f9c4 to 45167b6 Compare June 29, 2024 06:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for encoders and support ENCODER_MAP_ENABLE on Planck rev7 #23967

Fix for encoders and support ENCODER_MAP_ENABLE on Planck rev7 #23967

ykeisuke commented Jun 20, 2024 •

edited

Loading

tzarc Jun 24, 2024

ykeisuke Jun 25, 2024 •

edited

Loading

ykeisuke Jun 25, 2024

sigprof Jun 25, 2024

ykeisuke Jun 25, 2024

ykeisuke Jun 25, 2024

sigprof Jun 25, 2024

sigprof Jun 25, 2024

sigprof Jun 25, 2024

ykeisuke Jun 25, 2024

ykeisuke Jun 25, 2024

ykeisuke commented Jun 29, 2024

Fix for encoders and support ENCODER_MAP_ENABLE on Planck rev7 #23967

Are you sure you want to change the base?

Fix for encoders and support ENCODER_MAP_ENABLE on Planck rev7 #23967

Conversation

ykeisuke commented Jun 20, 2024 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

Choose a reason for hiding this comment

ykeisuke Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ykeisuke commented Jun 29, 2024

ykeisuke commented Jun 20, 2024 •

edited

Loading

ykeisuke Jun 25, 2024 •

edited

Loading