Change split_common to use RGBLIGHT_SPLIT (#5509)

* add I2C_slave_buffer_t to quantum/split_common/transport.c

Improvements to ease the maintenance of the I2C slave buffer layout. And this commit does not change the compilation results.

* add temporary pdhelix(Patched Helix) code

* temporary cherry-pick from #5020

add new version(#5020) quantum/rgblight.[ch], quantum/rgblight_modes.h

* add post_config.h support to build_keyboard.mk

* add quantum/rgblight_post_config.h, quantum/split_common/post_config.h

Add quantum/rgblight_post_config.h and quantum/split_common/post_config.h using POST_CONFIG_H variable of build_keyboard.mk.

quantum/rgblight_post_config.h additionally defines RGBLIGHT_SPLIT if RGBLED_SPIT is defined.

quantum/split_common/post_config.h defines RGBLIGHT_SPLIT additionally when master-slave communication is I2C.

* Change split_common's transport.c I2C to use the synchronization feature of rgblight.c

* Change split_common's transport.c serial to use the synchronization feature of rgblight.c

* test RGBLIGHT_SPLIT on keyboards/handwired/pdhelix

* Test End Revert "test RGBLIGHT_SPLIT on keyboards/handwired/pdhelix"

This reverts commit 80118a6bbd.

[x] make RGBLIGHT_TEST=1 handwired/pdhelix/i2c:default
[x] make RGBLIGHT_TEST=2 handwired/pdhelix/i2c:default (same RGBLIGHT_TEST=3)
[x] make RGBLIGHT_TEST=3 handwired/pdhelix/i2c:default

[x] make RGBLIGHT_TEST=1 handwired/pdhelix/pd2:default
[x] make RGBLIGHT_TEST=2 handwired/pdhelix/pd2:default
[x] make RGBLIGHT_TEST=3 handwired/pdhelix/pd2:default

[x] make RGBLIGHT_TEST=1 handwired/pdhelix/pd2_2oled:default
[x] make RGBLIGHT_TEST=2 handwired/pdhelix/pd2_2oled:default
[x] make RGBLIGHT_TEST=3 handwired/pdhelix/pd2_2oled:default

* Test End, Revert "temporary cherry-pick from #5020"

This reverts commit d35069f68b.

* Test End, Revert "add temporary pdhelix(Patched Helix) code"

This reverts commit aebddfc1a8.

* temporarily cherry-pick from #5020 to see if it passes the travis-ci test.

add new version(#5020) quantum/rgblight.[ch], quantum/rgblight_modes.h

* Passed the travis-ci test. Revert "temporarily cherry-pick from #5020 to see if it passes the travis-ci test."

This reverts commit 647c0a9755.

* update docs/config_options.md

* update split_common/transport.c, improves maintainability of serial transaction IDs.

No change in build result.

* temporary cherry-pick from #5020

* fix build fail keebio/iris/rev3:default

* fix build fail lets_split_eh/eh:default

* Revert "temporary cherry-pick from #5020"

This reverts commit be48ca1b45.

* temporary cherry-pick from #5020 (0.6.336)

* Revert "temporary cherry-pick from #5020 (0.6.336)"

This reverts commit 978d26a8b3.

* temporary cherry-pick from #5020 (0.6.336)

build_keyboard.mk

@@ -280,6 +280,23 @@ ifneq ("$(wildcard $(KEYBOARD_PATH_1)/config.h)","")
     CONFIG_H += $(KEYBOARD_PATH_1)/config.h
 endif
 
+POST_CONFIG_H :=
+ifneq ("$(wildcard $(KEYBOARD_PATH_1)/post_config.h)","")
+    POST_CONFIG_H += $(KEYBOARD_PATH_1)/post_config.h
+endif
+ifneq ("$(wildcard $(KEYBOARD_PATH_2)/post_config.h)","")
+    POST_CONFIG_H += $(KEYBOARD_PATH_2)/post_config.h
+endif
+ifneq ("$(wildcard $(KEYBOARD_PATH_3)/post_config.h)","")
+    POST_CONFIG_H += $(KEYBOARD_PATH_3)/post_config.h
+endif
+ifneq ("$(wildcard $(KEYBOARD_PATH_4)/post_config.h)","")
+    POST_CONFIG_H += $(KEYBOARD_PATH_4)/post_config.h
+endif
+ifneq ("$(wildcard $(KEYBOARD_PATH_5)/post_config.h)","")
+    POST_CONFIG_H += $(KEYBOARD_PATH_5)/post_config.h
+endif
+
 # Save the defines and includes here, so we don't include any keymap specific ones
 PROJECT_DEFS := $(OPT_DEFS)
 PROJECT_INC := $(VPATH) $(EXTRAINCDIRS) $(KEYBOARD_PATHS)
@@ -355,6 +372,7 @@ ifeq ($(strip $(VISUALIZER_ENABLE)), yes)
     include $(VISUALIZER_PATH)/visualizer.mk
 endif
 
+CONFIG_H += $(POST_CONFIG_H)
 ALL_CONFIGS := $(PROJECT_CONFIG) $(CONFIG_H)
 
 OUTPUTS := $(KEYMAP_OUTPUT) $(KEYBOARD_OUTPUT)

common_features.mk

@@ -103,6 +103,7 @@ ifeq ($(strip $(UNICODE_COMMON)), yes)
 endif
 
 ifeq ($(strip $(RGBLIGHT_ENABLE)), yes)
+    POST_CONFIG_H += $(QUANTUM_DIR)/rgblight_post_config.h
     OPT_DEFS += -DRGBLIGHT_ENABLE
     SRC += $(QUANTUM_DIR)/rgblight.c
     CIE1931_CURVE = yes
@@ -318,6 +319,7 @@ ifneq ($(strip $(DEBOUNCE_TYPE)), custom)
 endif
 
 ifeq ($(strip $(SPLIT_KEYBOARD)), yes)
+    POST_CONFIG_H += $(QUANTUM_DIR)/split_common/post_config.h
     OPT_DEFS += -DSPLIT_KEYBOARD
 
     # Include files used by all split keyboards

docs/config_options.md

@@ -180,10 +180,12 @@ If you define these options you will enable the associated feature, which may in
   * run RGB animations
 * `#define RGBLED_NUM 12`
   * number of LEDs
+* `#define RGBLIGHT_SPLIT`
+  * Needed if both halves of the board have RGB LEDs wired directly to the RGB output pin on the controllers instead of passing the output of the left half to the input of the right half
 * `#define RGBLED_SPLIT { 6, 6 }`
   * number of LEDs connected that are directly wired to `RGB_DI_PIN` on each half of a split keyboard
   * First value indicates number of LEDs for left half, second value is for the right half
-  * Needed if both halves of the board have RGB LEDs wired directly to the RGB output pin on the controllers instead of passing the output of the left half to the input of the right half
+  * When RGBLED_SPLIT is defined, RGBLIGHT_SPLIT is implicitly defined.
 * `#define RGBLIGHT_HUE_STEP 12`
   * units to step when in/decreasing hue
 * `#define RGBLIGHT_SAT_STEP 25`

quantum/rgblight_post_config.h

@@ -0,0 +1,5 @@
+#if defined(RGBLED_SPLIT) && !defined(RGBLIGHT_SPLIT)
+  // When RGBLED_SPLIT is defined,
+  // it is considered that RGBLIGHT_SPLIT is defined implicitly.
+  #define RGBLIGHT_SPLIT
+#endif

quantum/split_common/post_config.h

@@ -0,0 +1,15 @@
+#if defined(USE_I2C) || defined(EH)
+  // When using I2C, using rgblight implicitly involves split support.
+  #if defined(RGBLIGHT_ENABLE) && !defined(RGBLIGHT_SPLIT)
+    #define RGBLIGHT_SPLIT
+  #endif
+
+#else  // use serial
+  // When using serial, the user must define RGBLIGHT_SPLIT explicitly
+  //  in config.h as needed.
+  //      see quantum/rgblight_post_config.h
+  #if defined(RGBLIGHT_ENABLE) && defined(RGBLIGHT_SPLIT)
+    // When using serial and RGBLIGHT_SPLIT need separate transaction
+    #define SERIAL_USE_MULTI_TRANSACTION
+  #endif
+#endif

quantum/split_common/transport.c

@@ -25,36 +25,23 @@ extern backlight_config_t backlight_config;
 #  include "i2c_master.h"
 #  include "i2c_slave.h"
 
-typedef struct __attribute__ ((__packed__)) {
-#ifdef BACKLIGHT_ENABLE
-  uint8_t backlight_level;
-#endif
-#ifdef RGBLIGHT_ENABLE
-  uint32_t rgb_settings;
+typedef struct _I2C_slave_buffer_t {
+    matrix_row_t smatrix[ROWS_PER_HAND];
+    uint8_t      backlight_level;
+#if defined(RGBLIGHT_ENABLE) && defined(RGBLIGHT_SPLIT)
+    rgblight_syncinfo_t rgblight_sync;
 #endif
 #ifdef ENCODER_ENABLE
-  uint8_t encoder_state[NUMBER_OF_ENCODERS];
-#endif
-  // Keep matrix last, we are only using this for it's offset
-  uint8_t matrix_start[0];
-} transport_values_t;
-
-__attribute__ ((unused))
-static transport_values_t transport_values;
-
-#ifdef BACKLIGHT_ENABLE
-#  define I2C_BACKLIT_START (uint8_t)offsetof(transport_values_t, backlight_level)
-#endif
-
-#ifdef RGBLIGHT_ENABLE
-#  define I2C_RGB_START (uint8_t)offsetof(transport_values_t, rgb_settings)
+    uint8_t encoder_state[NUMBER_OF_ENCODERS];
 #endif
+} I2C_slave_buffer_t;
 
-#ifdef ENCODER_ENABLE
-#  define I2C_ENCODER_START (uint8_t)offsetof(transport_values_t, encoder_state)
-#endif
+static I2C_slave_buffer_t * const i2c_buffer = (I2C_slave_buffer_t *)i2c_slave_reg;
 
-#define I2C_KEYMAP_START (uint8_t)offsetof(transport_values_t, matrix_start)
+#  define I2C_BACKLIGHT_START offsetof(I2C_slave_buffer_t, backlight_level)
+#  define I2C_RGB_START offsetof(I2C_slave_buffer_t, rgblight_sync)
+#  define I2C_KEYMAP_START offsetof(I2C_slave_buffer_t, smatrix)
+#  define I2C_ENCODER_START offsetof(I2C_slave_buffer_t, encoder_state)
 
 #  define TIMEOUT 100
 
@@ -64,30 +51,32 @@ static transport_values_t transport_values;
 
 // Get rows from other half over i2c
 bool transport_master(matrix_row_t matrix[]) {
-  i2c_readReg(SLAVE_I2C_ADDRESS, I2C_KEYMAP_START, (void *)matrix, ROWS_PER_HAND * sizeof(matrix_row_t), TIMEOUT);
+  i2c_readReg(SLAVE_I2C_ADDRESS, I2C_KEYMAP_START, (void *)matrix, sizeof(i2c_buffer->smatrix), TIMEOUT);
 
   // write backlight info
 #  ifdef BACKLIGHT_ENABLE
   uint8_t level = get_backlight_level();
-  if (level != transport_values.backlight_level) {
-    if (i2c_writeReg(SLAVE_I2C_ADDRESS, I2C_BACKLIT_START, (void *)&level, sizeof(level), TIMEOUT) >= 0) {
-      transport_values.backlight_level = level;
+  if (level != i2c_buffer->backlight_level) {
+    if (i2c_writeReg(SLAVE_I2C_ADDRESS, I2C_BACKLIGHT_START, (void *)&level, sizeof(level), TIMEOUT) >= 0) {
+      i2c_buffer->backlight_level = level;
     }
   }
 #  endif
 
-#  ifdef RGBLIGHT_ENABLE
-  uint32_t rgb = rgblight_read_dword();
-  if (rgb != transport_values.rgb_settings) {
-    if (i2c_writeReg(SLAVE_I2C_ADDRESS, I2C_RGB_START, (void *)&rgb, sizeof(rgb), TIMEOUT) >= 0) {
-      transport_values.rgb_settings = rgb;
+#  if defined(RGBLIGHT_ENABLE) && defined(RGBLIGHT_SPLIT)
+  if (rgblight_get_change_flags()) {
+    rgblight_syncinfo_t rgblight_sync;
+    rgblight_get_syncinfo(&rgblight_sync);
+    if (i2c_writeReg(SLAVE_I2C_ADDRESS, I2C_RGB_START,
+                     (void *)&rgblight_sync, sizeof(rgblight_sync), TIMEOUT) >= 0) {
+      rgblight_clear_change_flags();
     }
   }
 #  endif
 
 #  ifdef ENCODER_ENABLE
-  i2c_readReg(SLAVE_I2C_ADDRESS, I2C_ENCODER_START, (void *)transport_values.encoder_state, sizeof(transport_values.encoder_state), TIMEOUT);
-  encoder_update_raw(&transport_values.encoder_state[0]);
+  i2c_readReg(SLAVE_I2C_ADDRESS, I2C_ENCODER_START, (void *)i2c_buffer->encoder_state, sizeof(I2C_slave_buffer_t.encoder_state), TIMEOUT);
+  encoder_update_raw(i2c_buffer->encoder_state);
 #  endif
 
   return true;
@@ -95,21 +84,23 @@ bool transport_master(matrix_row_t matrix[]) {
 
 void transport_slave(matrix_row_t matrix[]) {
   // Copy matrix to I2C buffer
-  memcpy((void*)(i2c_slave_reg + I2C_KEYMAP_START), (void *)matrix, ROWS_PER_HAND * sizeof(matrix_row_t) );
+  memcpy((void*)i2c_buffer->smatrix, (void *)matrix, sizeof(i2c_buffer->smatrix));
 
 // Read Backlight Info
 #  ifdef BACKLIGHT_ENABLE
-  backlight_set(i2c_slave_reg[I2C_BACKLIT_START]);
+  backlight_set(i2c_buffer->backlight_level);
 #  endif
 
-#  ifdef RGBLIGHT_ENABLE
-  uint32_t rgb = *(uint32_t *)(i2c_slave_reg + I2C_RGB_START);
+#  if defined(RGBLIGHT_ENABLE) && defined(RGBLIGHT_SPLIT)
   // Update the RGB with the new data
-  rgblight_update_dword(rgb);
+  if (i2c_buffer->rgblight_sync.status.change_flags != 0) {
+      rgblight_update_sync(&i2c_buffer->rgblight_sync, false);
+      i2c_buffer->rgblight_sync.status.change_flags = 0;
+  }
 #  endif
 
 #  ifdef ENCODER_ENABLE
-  encoder_state_raw((uint8_t*)(i2c_slave_reg + I2C_ENCODER_START));
+  encoder_state_raw(i2c_buffer->encoder_state);
 #  endif
 }
 
@@ -121,53 +112,109 @@ void transport_slave_init(void) { i2c_slave_init(SLAVE_I2C_ADDRESS); }
 
 #  include "serial.h"
 
-typedef struct __attribute__ ((__packed__)) {
+typedef struct _Serial_s2m_buffer_t {
+  // TODO: if MATRIX_COLS > 8 change to uint8_t packed_matrix[] for pack/unpack
+  matrix_row_t smatrix[ROWS_PER_HAND];
+
 #  ifdef ENCODER_ENABLE
   uint8_t encoder_state[NUMBER_OF_ENCODERS];
 #  endif
-  // TODO: if MATRIX_COLS > 8 change to uint8_t packed_matrix[] for pack/unpack
-  matrix_row_t smatrix[ROWS_PER_HAND];
+
 } Serial_s2m_buffer_t;
 
-typedef struct __attribute__ ((__packed__)) {
+typedef struct _Serial_m2s_buffer_t {
 #  ifdef BACKLIGHT_ENABLE
   uint8_t           backlight_level;
 #  endif
-#  if defined(RGBLIGHT_ENABLE) && defined(RGBLED_SPLIT)
-  rgblight_config_t rgblight_config;  // not yet use
-  //
-  // When MCUs on both sides drive their respective RGB LED chains,
-  // it is necessary to synchronize, so it is necessary to communicate RGB
-  // information. In that case, define RGBLED_SPLIT with info on the number
-  // of LEDs on each half.
-  //
-  // Otherwise, if the master side MCU drives both sides RGB LED chains,
-  // there is no need to communicate.
-#  endif
 } Serial_m2s_buffer_t;
 
+#if defined(RGBLIGHT_ENABLE) && defined(RGBLIGHT_SPLIT)
+// When MCUs on both sides drive their respective RGB LED chains,
+// it is necessary to synchronize, so it is necessary to communicate RGB
+// information. In that case, define RGBLIGHT_SPLIT with info on the number
+// of LEDs on each half.
+//
+// Otherwise, if the master side MCU drives both sides RGB LED chains,
+// there is no need to communicate.
+
+typedef struct _Serial_rgblight_t {
+    rgblight_syncinfo_t rgblight_sync;
+} Serial_rgblight_t;
+
+volatile Serial_rgblight_t serial_rgblight = {};
+uint8_t volatile status_rgblight           = 0;
+#endif
+
 volatile Serial_s2m_buffer_t serial_s2m_buffer = {};
 volatile Serial_m2s_buffer_t serial_m2s_buffer = {};
 uint8_t volatile status0                       = 0;
 
+enum serial_transaction_id {
+    GET_SLAVE_MATRIX = 0,
+#if defined(RGBLIGHT_ENABLE) && defined(RGBLIGHT_SPLIT)
+    PUT_RGBLIGHT,
+#endif
+};
+
 SSTD_t transactions[] = {
-    {
+    [GET_SLAVE_MATRIX] = {
         (uint8_t *)&status0,
         sizeof(serial_m2s_buffer),
         (uint8_t *)&serial_m2s_buffer,
         sizeof(serial_s2m_buffer),
         (uint8_t *)&serial_s2m_buffer,
     },
+#if defined(RGBLIGHT_ENABLE) && defined(RGBLIGHT_SPLIT)
+    [PUT_RGBLIGHT] = {
+        (uint8_t *)&status_rgblight,
+        sizeof(serial_rgblight),
+        (uint8_t *)&serial_rgblight,
+        0, NULL // no slave to master transfer
+    },
+#endif
 };
 
 void transport_master_init(void) { soft_serial_initiator_init(transactions, TID_LIMIT(transactions)); }
 
 void transport_slave_init(void) { soft_serial_target_init(transactions, TID_LIMIT(transactions)); }
 
+#if defined(RGBLIGHT_ENABLE) && defined(RGBLIGHT_SPLIT)
+
+// rgblight synchronization information communication.
+
+void transport_rgblight_master(void) {
+  if (rgblight_get_change_flags()) {
+    rgblight_get_syncinfo((rgblight_syncinfo_t *)&serial_rgblight.rgblight_sync);
+    if (soft_serial_transaction(PUT_RGBLIGHT) == TRANSACTION_END) {
+        rgblight_clear_change_flags();
+    }
+  }
+}
+
+void transport_rgblight_slave(void) {
+  if (status_rgblight == TRANSACTION_ACCEPTED) {
+    rgblight_update_sync((rgblight_syncinfo_t *)&serial_rgblight.rgblight_sync,
+                         false);
+    status_rgblight = TRANSACTION_END;
+  }
+}
+
+#else
+#define transport_rgblight_master()
+#define transport_rgblight_slave()
+#endif
+
 bool transport_master(matrix_row_t matrix[]) {
-  if (soft_serial_transaction()) {
+#ifndef SERIAL_USE_MULTI_TRANSACTION
+  if (soft_serial_transaction() != TRANSACTION_END) {
     return false;
   }
+#else
+  transport_rgblight_master();
+  if (soft_serial_transaction(GET_SLAVE_MATRIX) != TRANSACTION_END) {
+    return false;
+  }
+#endif
 
   // TODO:  if MATRIX_COLS > 8 change to unpack()
   for (int i = 0; i < ROWS_PER_HAND; ++i) {
@@ -179,23 +226,15 @@ bool transport_master(matrix_row_t matrix[]) {
   serial_m2s_buffer.backlight_level = backlight_config.enable ? backlight_config.level : 0;
 #  endif
 
-#  if defined(RGBLIGHT_ENABLE) && defined(RGBLED_SPLIT)
-  static rgblight_config_t prev_rgb = {~0};
-  uint32_t rgb = rgblight_read_dword();
-  if (rgb != prev_rgb.raw) {
-    serial_m2s_buffer.rgblight_config.raw = rgb;
-    prev_rgb.raw = rgb;
-  }
-#  endif
-
 #  ifdef ENCODER_ENABLE
-  encoder_update_raw((uint8_t*)&serial_s2m_buffer.encoder_state);
+  encoder_update_raw((uint8_t *)serial_s2m_buffer.encoder_state);
 #  endif
 
   return true;
 }
 
 void transport_slave(matrix_row_t matrix[]) {
+  transport_rgblight_slave();
   // TODO: if MATRIX_COLS > 8 change to pack()
   for (int i = 0; i < ROWS_PER_HAND; ++i) {
     serial_s2m_buffer.smatrix[i] = matrix[i];
@@ -203,14 +242,11 @@ void transport_slave(matrix_row_t matrix[]) {
 #  ifdef BACKLIGHT_ENABLE
   backlight_set(serial_m2s_buffer.backlight_level);
 #  endif
-#  if defined(RGBLIGHT_ENABLE) && defined(RGBLED_SPLIT)
-  // Update RGB config with the new data
-  rgblight_update_dword(serial_m2s_buffer.rgblight_config.raw);
-#  endif
 
 #  ifdef ENCODER_ENABLE
-  encoder_state_raw((uint8_t*)&serial_s2m_buffer.encoder_state);
+  encoder_state_raw((uint8_t *)serial_s2m_buffer.encoder_state);
 #  endif
+
 }
 
 #endif