Fix encoder_init call order in keyboard_init

[chrishoage]

Description

From the insight of sigprof it was discovered that encoder_init initializes thisCount which is used in encoder_state_raw memcpy. quantum_init was calling encoder_state_raw though the initialization of bootmatgic calling matrix_scan. This caused undefined behavior resulting in bad values being sent over transport. Adjusting
the call order of encoder_init allows for correct behavior of encoder_state_raw

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] encoder_update_raw called with incorrect values in some cases  #19139

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

[chrishoage]

As mentioned I'm happy to adjust my approach.

In my investigation slave_state was receiving values of 17 and 46 for both from the slave, then alter values of 0 and 46 (as the right side is master for me and thus would not change)

I have no idea where the 17 value is coming from. Doing a full memcopy of encoder_value makes my issue go away entirely.

[chrishoage]

I realized a while after I made this change that it will obviously break the encoder_update_raw logic due to index mismatch.

I spent a little more time playing around and noted that

static uint8_t encoder_state[NUM_ENCODERS_MAX_PER_SIDE]; and uint8_t encoder_state[NUM_ENCODERS_MAX_PER_SIDE] = {0}; in encoder_handlers_slave

both fixed the issue as well

[chrishoage]

I opted to go with uint8_t encoder_state[NUM_ENCODERS_MAX_PER_SIDE] = {0}; in encoder_handlers_slave.

All unit tests pass with out modification, and this reliably fixes the issue.

I have tested by flashing my slave half with this change and my master half with out this change.

My moving the usb cable between the halfs I get a burst on the slave and no burst on the right.

I'd really love to understand why this fixes the issue. My current uneducated guess is that split_shmem->encoders.state contains something unexpected (perhaps due to trans_target2initiator_initializer?) which gets copied over to master. encoder_state_raw is later run and this gets the value back to 0 from the previously undefined value.

[sigprof]

Hmm, there might be some problems in the initialization order:

• keyboard_init()
  • …
  • quantum_init()
    • magic()
      • bootmagic()
        • matrix_scan()
          • transport_master() or transport_slave()
            • …
              • encoder_state_raw() — here thisCount is not initialized yet, so memcpy() does nothing, leaving the returned encoder state uninitialized
  • …
  • encoder_init() — this initializes thisCount and other similar variables in encoder.c

[chrishoage]

@sigprof That is a great insight! I can confrim that moving encoder_init() above quantum_init() in keyboard_init addresses the issue.

Would this be preferable? If so I can update my PR here with the adjustment to the call order in keyboard_init.

diff --git a/quantum/keyboard.c b/quantum/keyboard.c
index 83ade7829a..37675ded0b 100644
--- a/quantum/keyboard.c
+++ b/quantum/keyboard.c
@@ -353,6 +353,9 @@ void keyboard_init(void) {
 #endif
 #ifdef SPLIT_KEYBOARD
     split_pre_init();
+#endif
+#ifdef ENCODER_ENABLE
+    encoder_init();
 #endif
     matrix_init();
     quantum_init();
@@ -374,9 +377,6 @@ void keyboard_init(void) {
 #ifdef RGBLIGHT_ENABLE
     rgblight_init();
 #endif
-#ifdef ENCODER_ENABLE
-    encoder_init();
-#endif
 #ifdef STENO_ENABLE_ALL
     steno_init();
 #endif

[tzarc]

Would this be preferable? If so I can update my PR here with the adjustment to the call order in keyboard_init.

Yes, please do so.

[chrishoage]

@tzarc I have updated the diff to change the call order of encoder_init along with updated the PR title and body and commit message