Extensible split data sync (qmk#11930)

* Extensible split data sync capability through transactions.

- Split common transport has been split up between the transport layer
  and data layer.
- Split "transactions" model used, with convergence between I2C and
  serial data definitions.
- Slave matrix "generation count" is used to determine if the full slave
  matrix needs to be retrieved.
- Encoders get the same "generation count" treatment.
- All other blocks of data are synchronised when a change is detected.
- All transmissions have a globally-configurable deadline before a
  transmission is forced (`FORCED_SYNC_THROTTLE_MS`, default 100ms).
- Added atomicity for all core-synced data, preventing partial updates
- Added retries to AVR i2c_master's i2c_start, to minimise the number of
  failed transactions when interrupts are disabled on the slave due to
  atomicity checks.
- Some keyboards have had slight modifications made in order to ensure
  that they still build due to firmware size restrictions.

* Fixup LED_MATRIX compile.

* Parameterise ERROR_DISCONNECT_COUNT.

Author: tzarc

common_features.mk

@@ -524,7 +524,11 @@ ifeq ($(strip $(SPLIT_KEYBOARD)), yes)
 
     # Determine which (if any) transport files are required
     ifneq ($(strip $(SPLIT_TRANSPORT)), custom)
-        QUANTUM_LIB_SRC += $(QUANTUM_DIR)/split_common/transport.c
+        QUANTUM_SRC += $(QUANTUM_DIR)/split_common/transport.c \
+                       $(QUANTUM_DIR)/split_common/transactions.c
+
+        OPT_DEFS += -DSPLIT_COMMON_TRANSACTIONS
+
         # Functions added via QUANTUM_LIB_SRC are only included in the final binary if they're called.
         # Unused functions are pruned away, which is why we can add multiple drivers here without bloat.
         ifeq ($(PLATFORM),AVR)

docs/config_options.md

@@ -284,7 +284,7 @@ There are a few different ways to set handedness for split keyboards (listed in
 ### Other Options
 
 * `#define USE_I2C`
-  * For using I2C instead of Serial (defaults to serial)
+  * For using I2C instead of Serial (default is serial; serial transport is supported on ARM -- I2C is AVR-only)
 
 * `#define SOFT_SERIAL_PIN D0`
   * When using serial, define this. `D0` or `D1`,`D2`,`D3`,`E6`.
@@ -312,14 +312,36 @@ There are a few different ways to set handedness for split keyboards (listed in
 * `#define SPLIT_USB_DETECT`
   * Detect (with timeout) USB connection when delegating master/slave
   * Default behavior for ARM
-  * Required for AVR Teensy
+  * Required for AVR Teensy (without hardware mods)
 
 * `#define SPLIT_USB_TIMEOUT 2000`
   * Maximum timeout when detecting master/slave when using `SPLIT_USB_DETECT`
 
 * `#define SPLIT_USB_TIMEOUT_POLL 10`
   * Poll frequency when detecting master/slave when using `SPLIT_USB_DETECT`
 
+* `#define FORCED_SYNC_THROTTLE_MS 100`
+  * Deadline for synchronizing data from master to slave when using the QMK-provided split transport.
+
+* `#define SPLIT_TRANSPORT_MIRROR`
+  * Mirrors the master-side matrix on the slave when using the QMK-provided split transport.
+
+* `#define SPLIT_LAYER_STATE_ENABLE`
+  * Ensures the current layer state is available on the slave when using the QMK-provided split transport.
+
+* `#define SPLIT_LED_STATE_ENABLE`
+  * Ensures the current host indicator state (caps/num/scroll) is available on the slave when using the QMK-provided split transport.
+
+* `#define SPLIT_MODS_ENABLE`
+  * Ensures the current modifier state (normal, weak, and oneshot) is available on the slave when using the QMK-provided split transport.
+
+* `#define SPLIT_WPM_ENABLE`
+  * Ensures the current WPM is available on the slave when using the QMK-provided split transport.
+
+* `#define SPLIT_TRANSACTION_IDS_KB .....`
+* `#define SPLIT_TRANSACTION_IDS_USER .....`
+  * Allows for custom data sync with the slave when using the QMK-provided split transport. See [custom data sync between sides](feature_split_keyboard.md#custom-data-sync) for more information.
+
 # The `rules.mk` File
 
 This is a [make](https://www.gnu.org/software/make/manual/make.html) file that is included by the top-level `Makefile`. It is used to set some information about the MCU that we will be compiling for as well as enabling and disabling certain features.

docs/feature_split_keyboard.md

@@ -8,8 +8,7 @@ QMK Firmware has a generic implementation that is usable by any board, as well a
 
 For this, we will mostly be talking about the generic implementation used by the Let's Split and other keyboards. 
 
-!> ARM is not yet fully supported for Split Keyboards and has many limitations. Progress is being made, but we have not yet reached 100% feature parity.
-
+!> ARM split supports most QMK subsystems when using the 'serial' and 'serial_usart' drivers. I2C slave is currently unsupported.
 
 ## Compatibility Overview
 
@@ -168,7 +167,7 @@ Because not every split keyboard is identical, there are a number of additional
 #define USE_I2C
 ```
 
-This enables I<sup>2</sup>C support for split keyboards. This isn't strictly for communication, but can be used for OLED or other I<sup>2</sup>C-based devices. 
+This configures the use of I<sup>2</sup>C support for split keyboard transport (AVR only).  
 
 ```c
 #define SOFT_SERIAL_PIN D0
@@ -192,20 +191,115 @@ If you're having issues with serial communication, you can change this value, as
 * **`5`**: about 20kbps
 
 ```c
-#define SPLIT_MODS_ENABLE
+#define FORCED_SYNC_THROTTLE_MS 100
 ```
 
-This enables transmitting modifier state (normal, weak and oneshot) to the non
-primary side of the split keyboard.  This adds a few bytes of data to the split
-communication protocol and may impact the matrix scan speed when enabled.
-The purpose of this feature is to support cosmetic use of modifer state (e.g.
-displaying status on an OLED screen).
+This sets the maximum number of milliseconds before forcing a synchronization of data from master to slave. Under normal circumstances this sync occurs whenever the data _changes_, for safety a data transfer occurs after this number of milliseconds if no change has been detected since the last sync. 
 
 ```c
 #define SPLIT_TRANSPORT_MIRROR
 ```
 
-This mirrors the master side matrix to the slave side for features that react or require knowledge of master side key presses on the slave side.  This adds a few bytes of data to the split communication protocol and may impact the matrix scan speed when enabled. The purpose of this feature is to support cosmetic use of key events (e.g. RGB reacting to Keypresses).
+This mirrors the master side matrix to the slave side for features that react or require knowledge of master side key presses on the slave side. The purpose of this feature is to support cosmetic use of key events (e.g. RGB reacting to keypresses). This adds overhead to the split communication protocol and may negatively impact the matrix scan speed when enabled. 
+
+```c
+#define SPLIT_LAYER_STATE_ENABLE
+```
+
+This enables syncing of the layer state between both halves of the split keyboard. The main purpose of this feature is to enable support for use of things like OLED display of the currently active layer. This adds overhead to the split communication protocol and may negatively impact the matrix scan speed when enabled. 
+
+```c
+#define SPLIT_LED_STATE_ENABLE
+```
+
+This enables syncing of the Host LED status (caps lock, num lock, etc) between both halves of the split keyboard. The main purpose of this feature is to enable support for use of things like OLED display of the Host LED status. This adds overhead to the split communication protocol and may negatively impact the matrix scan speed when enabled. 
+
+```c
+#define SPLIT_MODS_ENABLE
+```
+
+This enables transmitting modifier state (normal, weak and oneshot) to the non primary side of the split keyboard. The purpose of this feature is to support cosmetic use of modifer state (e.g. displaying status on an OLED screen). This adds overhead to the split communication protocol and may negatively impact the matrix scan speed when enabled. 
+
+```c
+#define SPLIT_WPM_ENABLE
+```
+
+This enables transmitting the current WPM to the slave side of the split keyboard. The purpose of this feature is to support cosmetic use of WPM (e.g. displaying the current value on an OLED screen). This adds overhead to the split communication protocol and may negatively impact the matrix scan speed when enabled. 
+
+### Custom data sync between sides :id=custom-data-sync
+
+QMK's split transport allows for arbitrary data transactions at both the keyboard and user levels. This is modelled on a remote procedure call, with the master invoking a function on the slave side, with the ability to send data from master to slave, process it slave side, and send data back from slave to master.
+
+To leverage this, a keyboard or user/keymap can define a comma-separated list of _transaction IDs_:
+
+```c
+// for keyboard-level data sync:
+#define SPLIT_TRANSACTION_IDS_KB KEYBOARD_SYNC_A, KEYBOARD_SYNC_B
+// or, for user:
+#define SPLIT_TRANSACTION_IDS_USER USER_SYNC_A, USER_SYNC_B, USER_SYNC_C
+```
+
+These _transaction IDs_ then need a slave-side handler function to be registered with the split transport, for example:
+
+```c
+typedef struct _master_to_slave_t {
+    int m2s_data;
+} master_to_slave_t;
+
+typedef struct _slave_to_master_t {
+    int s2m_data;
+} slave_to_master_t;
+
+void user_sync_a_slave_handler(uint8_t in_buflen, const void* in_data, uint8_t out_buflen, void* out_data) {
+    const master_to_slave_t *m2s = (const master_to_slave_t*)in_data;
+    slave_to_master_t *s2m = (slave_to_master_t*)out_data;
+    s2m->s2m_data = m2s->m2s_data + 5; // whatever comes in, add 5 so it can be sent back
+}
+
+void keyboard_post_init_user(void) {
+    transaction_register_rpc(USER_SYNC_A, user_sync_a_slave_handler);
+}
+```
+
+The master side can then invoke the slave-side handler - for normal keyboard functionality to be minimally affected, any keyboard- or user-level code attempting to sync data should be throttled:
+
+```c
+void housekeeping_task_user(void) {
+    if (is_keyboard_master()) {
+        // Interact with slave every 500ms
+        static uint32_t last_sync = 0;
+        if (timer_elapsed32(last_sync) > 500) {
+            master_to_slave_t m2s = {6};
+            slave_to_master_t s2m = {0};
+            if(transaction_rpc_exec(USER_SYNC_A, sizeof(m2s), &m2s, sizeof(s2m), &s2m)) {
+                last_sync = timer_read32();
+                dprintf("Slave value: %d\n", s2m.s2m_data); // this will now be 11, as the slave adds 5
+            } else {
+                dprint("Slave sync failed!\n");
+            }
+        }
+    }
+}
+```
+
+!> It is recommended that any data sync between halves happens during the master side's _housekeeping task_. This ensures timely retries should failures occur.
+
+If only one-way data transfer is needed, helper methods are provided:
+
+```c
+bool transaction_rpc_exec(int8_t transaction_id, uint8_t initiator2target_buffer_size, const void *initiator2target_buffer, uint8_t target2initiator_buffer_size, void *target2initiator_buffer);
+bool transaction_rpc_send(int8_t transaction_id, uint8_t initiator2target_buffer_size, const void *initiator2target_buffer);
+bool transaction_rpc_recv(int8_t transaction_id, uint8_t target2initiator_buffer_size, void *target2initiator_buffer);
+```
+
+By default, the inbound and outbound data is limited to a maximum of 32 bytes each. The sizes can be altered if required:
+
+```c
+// Master to slave:
+#define RPC_M2S_BUFFER_SIZE 48
+// Slave to master:
+#define RPC_S2M_BUFFER_SIZE 48
+```
 
 ###  Hardware Configuration Options
 

drivers/avr/i2c_master.c

@@ -28,8 +28,14 @@
 #    define F_SCL 400000UL  // SCL frequency
 #endif
 
+#ifndef I2C_START_RETRY_COUNT
+#    define I2C_START_RETRY_COUNT 20
+#endif  // I2C_START_RETRY_COUNT
+
 #define TWBR_val (((F_CPU / F_SCL) - 16) / 2)
 
+#define MAX(X, Y) ((X) > (Y) ? (X) : (Y))
+
 void i2c_init(void) {
     TWSR = 0; /* no prescaler */
     TWBR = (uint8_t)TWBR_val;
@@ -47,7 +53,7 @@ void i2c_init(void) {
 #endif
 }
 
-i2c_status_t i2c_start(uint8_t address, uint16_t timeout) {
+static i2c_status_t i2c_start_impl(uint8_t address, uint16_t timeout) {
     // reset TWI control register
     TWCR = 0;
     // transmit START condition
@@ -86,6 +92,17 @@ i2c_status_t i2c_start(uint8_t address, uint16_t timeout) {
     return I2C_STATUS_SUCCESS;
 }
 
+i2c_status_t i2c_start(uint8_t address, uint16_t timeout) {
+    // Retry i2c_start_impl a bunch times in case the remote side has interrupts disabled.
+    uint16_t     timeout_timer = timer_read();
+    uint16_t     time_slice    = MAX(1, (timeout == (I2C_TIMEOUT_INFINITE)) ? 5 : (timeout / (I2C_START_RETRY_COUNT)));  // if it's infinite, wait 1ms between attempts, otherwise split up the entire timeout into the number of retries
+    i2c_status_t status;
+    do {
+        status = i2c_start_impl(address, time_slice);
+    } while ((status < 0) && ((timeout == I2C_TIMEOUT_INFINITE) || (timer_elapsed(timeout_timer) < timeout)));
+    return status;
+}
+
 i2c_status_t i2c_write(uint8_t data, uint16_t timeout) {
     // load data into data register
     TWDR = data;

drivers/avr/i2c_slave.c

@@ -17,13 +17,20 @@
  * GitHub repository: https://github.com/g4lvanix/I2C-slave-lib
  */
 
+#include <stddef.h>
 #include <avr/io.h>
 #include <util/twi.h>
 #include <avr/interrupt.h>
 #include <stdbool.h>
 
 #include "i2c_slave.h"
 
+#if defined(USE_I2C) && defined(SPLIT_COMMON_TRANSACTIONS)
+#    include "transactions.h"
+
+static volatile bool is_callback_executor = false;
+#endif  // defined(USE_I2C) && defined(SPLIT_COMMON_TRANSACTIONS)
+
 volatile uint8_t i2c_slave_reg[I2C_SLAVE_REG_COUNT];
 
 static volatile uint8_t buffer_address;
@@ -48,22 +55,40 @@ ISR(TWI_vect) {
         case TW_SR_SLA_ACK:
             // The device is now a slave receiver
             slave_has_register_set = false;
+#if defined(USE_I2C) && defined(SPLIT_COMMON_TRANSACTIONS)
+            is_callback_executor = false;
+#endif  // defined(USE_I2C) && defined(SPLIT_COMMON_TRANSACTIONS)
             break;
 
         case TW_SR_DATA_ACK:
             // This device is a slave receiver and has received data
-            // First byte is the location then the bytes will be writen in buffer with auto-incriment
+            // First byte is the location then the bytes will be writen in buffer with auto-increment
             if (!slave_has_register_set) {
                 buffer_address = TWDR;
 
                 if (buffer_address >= I2C_SLAVE_REG_COUNT) {  // address out of bounds dont ack
                     ack            = 0;
                     buffer_address = 0;
                 }
-                slave_has_register_set = true;  // address has been receaved now fill in buffer
+                slave_has_register_set = true;  // address has been received now fill in buffer
+
+#if defined(USE_I2C) && defined(SPLIT_COMMON_TRANSACTIONS)
+                // Work out if we're attempting to execute a callback
+                is_callback_executor = buffer_address == split_transaction_table[I2C_EXECUTE_CALLBACK].initiator2target_offset;
+#endif  // defined(USE_I2C) && defined(SPLIT_COMMON_TRANSACTIONS)
             } else {
                 i2c_slave_reg[buffer_address] = TWDR;
                 buffer_address++;
+
+#if defined(USE_I2C) && defined(SPLIT_COMMON_TRANSACTIONS)
+                // If we're intending to execute a transaction callback, do so, as we've just received the transaction ID
+                if (is_callback_executor) {
+                    split_transaction_desc_t *trans = &split_transaction_table[split_shmem->transaction_id];
+                    if (trans->slave_callback) {
+                        trans->slave_callback(trans->initiator2target_buffer_size, split_trans_initiator2target_buffer(trans), trans->target2initiator_buffer_size, split_trans_target2initiator_buffer(trans));
+                    }
+                }
+#endif  // defined(USE_I2C) && defined(SPLIT_COMMON_TRANSACTIONS)
             }
             break;
 

drivers/avr/i2c_slave.h

@@ -22,7 +22,18 @@
 
 #pragma once
 
-#define I2C_SLAVE_REG_COUNT 30
+#ifndef I2C_SLAVE_REG_COUNT
+
+#    if defined(USE_I2C) && defined(SPLIT_COMMON_TRANSACTIONS)
+#        include "transport.h"
+#        define I2C_SLAVE_REG_COUNT sizeof(split_shared_memory_t)
+#    else  // defined(USE_I2C) && defined(SPLIT_COMMON_TRANSACTIONS)
+#        define I2C_SLAVE_REG_COUNT 30
+#    endif  // defined(USE_I2C) && defined(SPLIT_COMMON_TRANSACTIONS)
+
+#endif  // I2C_SLAVE_REG_COUNT
+
+_Static_assert(I2C_SLAVE_REG_COUNT < 256, "I2C target registers must be single byte");
 
 extern volatile uint8_t i2c_slave_reg[I2C_SLAVE_REG_COUNT];