Extensible split data sync #11930

tzarc · 2021-02-16T01:47:11Z

Description

Adds support for keyboards and keymaps to define new split serial "transactions" -- data sharing between sides.

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.

Types of Changes

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

quantum/split_common/transport.c

tzarc · 2021-02-16T01:54:50Z

Might see if I can work out a generic solution for I2C in the process.

drashna · 2021-02-16T18:21:36Z

Is angry about the last changes:

quantum/split_common/transport.c: In function 'split_sync_register_transaction':
quantum/split_common/transport.c:358:56: error: array subscript is above array bounds [-Werror=array-bounds]
     SSTD_t *       trans                = &transactions[NUM_SERIAL_TRANSACTIONS + id];

tzarc · 2021-02-16T19:54:35Z

Is angry about the last changes:

quantum/split_common/transport.c: In function 'split_sync_register_transaction':
quantum/split_common/transport.c:358:56: error: array subscript is above array bounds [-Werror=array-bounds]
     SSTD_t *       trans                = &transactions[NUM_SERIAL_TRANSACTIONS + id];

In the current absence of docs, have you added this to your config.h?

#define SERIAL_USE_MULTI_TRANSACTION
#define SPLIT_NUM_TRANSACTIONS_USER 1

If you're using I2C, you'll also need:

#define I2C_SLAVE_USER_REG_COUNT ???

...replacing the ??? with the number of bytes used in the shared data.

drashna · 2021-02-17T11:30:37Z

Tested with serial, works.

Though, sounds like this requires multi-transaction, and if so... may want/need to make that the default or if-def the hell out of it

tzarc · 2021-02-17T22:38:21Z

Though, sounds like this requires multi-transaction, and if so... may want/need to make that the default or if-def the hell out of it

That's kinda my plan -- the intention is to decouple things like WPM and the like so that we can throttle propagation of less-frequently-updated data instead of during every matrix scan. I see no need to have non-transaction support.

mtei · 2021-02-18T09:11:23Z

the intention is to decouple things like WPM and the like so that we can throttle propagation of less-frequently-updated data instead of during every matrix scan.

I'm all for it. To be honest, I didn't like the fact that all matrix scans sent something other than the matrix data with them.

mtei · 2021-02-18T09:33:35Z

In the current absence of docs, have you added this to your config.h?
#define SERIAL_USE_MULTI_TRANSACTION
#define SPLIT_NUM_TRANSACTIONS_USER 1

For example:

user config.h:

#define SPLIT_NUM_TRANSACTIONS_USER 1

and change quantum/split_common/post_config.h:

--- a/quantum/split_common/post_config.h
+++ b/quantum/split_common/post_config.h
@@ -9,11 +9,9 @@
 #    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
+#    define SERIAL_USE_MULTI_TRANSACTION
+#    if !defined(SPLIT_NUM_TRANSACTIONS_USER) && \
+        !(defined(RGBLIGHT_ENABLE) && defined(RGBLIGHT_SPLIT))
+#        undef SERIAL_USE_MULTI_TRANSACTION
 #    endif
 #endif

drashna · 2021-02-19T06:16:39Z

the intention is to decouple things like WPM and the like so that we can throttle propagation of less-frequently-updated data instead of during every matrix scan.

I'm all for it. To be honest, I didn't like the fact that all matrix scans sent something other than the matrix data with them.

Honestly, I suspect that a lot of that is solely due to people not knowing how the multi-transactional part works. Myself included, actually. But this has gotten me to learn more of it,

Though, sounds like this requires multi-transaction, and if so... may want/need to make that the default or if-def the hell out of it

That's kinda my plan -- the intention is to decouple things like WPM and the like so that we can throttle propagation of less-frequently-updated data instead of during every matrix scan. I see no need to have non-transaction support.

Awesome! And yea, I think that's the best direction.

I'd also like to add layer (both normal and default layer) support well, and as 32bit bitmasks, transactional would probably be best for them, too.

drashna · 2021-02-19T06:18:48Z

and change quantum/split_common/post_config.h:

Honestly, I think that if there isn't a negative impact, that we should default to multi transactional framework.

tzarc · 2021-02-19T06:19:37Z

and change quantum/split_common/post_config.h:

Honestly, I think that if there isn't a negative impact, that we should default to multi transactional framework.

I'm inclined to remove non-transactional.

tzarc · 2021-02-19T06:20:58Z

I'd also like to add layer (both normal and default layer) support well, and as 32bit bitmasks, transactional would probably be best for them, too.

Yep, definitely - at the moment I'm sending layer information across fro master to slave once a second, OR if the layer state changes. This should mean that the worst case scenario is that they're out of sync for 1 second maximum, but in practice I've not seen any desync's at all.

tzarc · 2021-02-19T06:22:14Z

Example usage of maintaining shared state for both layer state and indicator LED state:
https://github.com/tzarc/qmk_build/blob/c3089b34a6532c52e80d109a23c9d7320b331807/tzarc-djinn/keymaps/default/keymap.c#L148-L217

mtei · 2021-02-19T22:48:39Z

For example, enum serial_transaction_id:

diff --git a/quantum/split_common/transaction_id_define.h b/quantum/split_common/transaction_id_define.h
new file mode 100644
index 000000000..d3ff96982
--- /dev/null
+++ b/quantum/split_common/transaction_id_define.h
@@ -0,0 +1,15 @@
+#pragma once
+
+enum serial_transaction_id {
+    GET_SLAVE_MATRIX = 0,
+#if defined(RGBLIGHT_ENABLE) && defined(RGBLIGHT_SPLIT)
+    PUT_RGBLIGHT,
+#endif
+#if defined(TRANSACTION_ID_KB)
+    TRANSACTION_ID_KB
+#endif
+#if defined(TRANSACTION_ID_USER)
+    TRANSACTION_ID_USER
+#endif
+    NUM_transaction_id
+};
diff --git a/quantum/split_common/transport.c b/quantum/split_common/transport.c
index 467ff81a9..cdd6f3804 100644
--- a/quantum/split_common/transport.c
+++ b/quantum/split_common/transport.c
@@ -4,6 +4,7 @@
 #include "config.h"
 #include "matrix.h"
 #include "quantum.h"
+#include "transaction_id_define.h"
 
 #define ROWS_PER_HAND (MATRIX_ROWS / 2)
 
@@ -168,14 +169,7 @@ 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[] = {
+SSTD_t transactions[NUM_transaction_id] = {
     [GET_SLAVE_MATRIX] =
         {
             (uint8_t *)&status0,

keyboard/user config.h:

diff --git a/keyboards/helix/rev3_5rows/config.h b/keyboards/helix/rev3_5rows/config.h
index 4dda76206..15cc11efa 100644
--- a/keyboards/helix/rev3_5rows/config.h
+++ b/keyboards/helix/rev3_5rows/config.h
@@ -241,3 +241,5 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 /* Bootmagic Lite key configuration */
 // #define BOOTMAGIC_LITE_ROW 0
 // #define BOOTMAGIC_LITE_COLUMN 0
+
+#define TRANSACTION_ID_KB   TRANS_ID_KB_1, TRANS_ID_KB_2,

and keyboard.c/keymap.c:

diff --git a/keyboards/helix/rev3_5rows/rev3_5rows.c b/keyboards/helix/rev3_5rows/rev3_5rows.c
index c034c8338..33690289c 100644
--- a/keyboards/helix/rev3_5rows/rev3_5rows.c
+++ b/keyboards/helix/rev3_5rows/rev3_5rows.c
@@ -15,6 +15,7 @@
  */
 
 #include "helix.h"
+#include <transaction_id_define.h>

tzarc · 2021-02-27T20:32:22Z

Sorry about that, GitHub decided to delete the develop branch from upstream when we merged it, despite being told otherwise. Reopened.

tzarc · 2021-02-28T21:17:19Z

For example, enum serial_transaction_id:

Oooh, yeah, I like this method better.

This reverts commit 172e6a7.

In qmk#11930 the I2C master driver for AVR was changed to perform multiple retries (20 by default, configurable via `I2C_START_RETRY_COUNT`) in `i2c_start()`, apparently as a workaround for failures when the slave half had interrupts disabled for some time. Unfortunately, the implementation of those retries limited the minimum timeout for a single start attempt to 1 ms, but the timeout handling in the I2C master driver handles the partial millisecond before the next timer tick as if it was a full millisecond, therefore the timeout for a single attempt could be really short in some cases. These short timeouts resulted in some problems when various I2C APIs were invoked with timeouts not greater than 40 ms - e.g., qmk#14935. As a minimal fix for this problem, limit the minimum timeout for a single start attempt to 2 ms instead of 1 (the actual timeout will be 1...2 ms, instead of 0...1 ms before the change).

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

* Unite half-duplex and full-duplex serial driver. * Add full duplex operation mode to the interrupt based driver * Delete DMA UART based full duplex driver * The new driver targets qmk#11930 * Fix freezes with failing transactions in half-duplex * Increase default serial TX/RX buffer size to 128 bytes * Correctly use bool instead of size_t Co-authored-by: Nick Brassel <[email protected]>

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

* Unite half-duplex and full-duplex serial driver. * Add full duplex operation mode to the interrupt based driver * Delete DMA UART based full duplex driver * The new driver targets qmk#11930 * Fix freezes with failing transactions in half-duplex * Increase default serial TX/RX buffer size to 128 bytes * Correctly use bool instead of size_t Co-authored-by: Nick Brassel <[email protected]>

tzarc requested a review from a team February 16, 2021 01:47

github-actions bot added the core label Feb 16, 2021

drashna reviewed Feb 16, 2021

View reviewed changes

quantum/split_common/transport.c Outdated Show resolved Hide resolved

tzarc force-pushed the extensible-serial-transactions branch from 2c7bf73 to b59de50 Compare February 16, 2021 03:09

tzarc changed the title ~~Extensible split data sync over serial~~ Extensible split data sync Feb 16, 2021

tzarc force-pushed the extensible-serial-transactions branch 2 times, most recently from c348933 to 4b84050 Compare February 16, 2021 12:26

mtei added the enhancement label Feb 18, 2021

drashna mentioned this pull request Feb 24, 2021

Split RGB Matrix #11055

Merged

14 tasks

tzarc closed this Feb 27, 2021

tzarc deleted the branch qmk:develop February 27, 2021 20:28

tzarc reopened this Feb 27, 2021

tzarc force-pushed the extensible-serial-transactions branch from 4b84050 to e1bb375 Compare March 2, 2021 20:40

drashna added a commit to drashna/qmk_firmware that referenced this pull request Jul 27, 2021

Revert "Extensible split data sync (qmk#11930)"

9743318

This reverts commit 172e6a7.

tzarc mentioned this pull request Aug 15, 2021

Add a lot more data to info.json #13366

Merged

6 tasks

mtei mentioned this pull request Sep 1, 2021

Re-Unify Split Keyboard Code and add ARM support #4254

Closed

12 tasks

manna-harbour mentioned this pull request Dec 4, 2022

fix: build errors manna-harbour/miryoku_qmk#12

Closed

manna-harbour mentioned this pull request Dec 15, 2022

Remove LAYER_STATE_8BIT manna-harbour/miryoku_qmk#13

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extensible split data sync #11930

Extensible split data sync #11930

tzarc commented Feb 16, 2021 •

edited

Loading

tzarc commented Feb 16, 2021

drashna commented Feb 16, 2021

tzarc commented Feb 16, 2021 •

edited

Loading

drashna commented Feb 17, 2021

tzarc commented Feb 17, 2021

mtei commented Feb 18, 2021

mtei commented Feb 18, 2021

drashna commented Feb 19, 2021

drashna commented Feb 19, 2021

tzarc commented Feb 19, 2021

tzarc commented Feb 19, 2021

tzarc commented Feb 19, 2021

mtei commented Feb 19, 2021

tzarc commented Feb 27, 2021

tzarc commented Feb 28, 2021

Extensible split data sync #11930

Extensible split data sync #11930

Conversation

tzarc commented Feb 16, 2021 • edited Loading

Description

Types of Changes

Checklist

tzarc commented Feb 16, 2021

drashna commented Feb 16, 2021

tzarc commented Feb 16, 2021 • edited Loading

drashna commented Feb 17, 2021

tzarc commented Feb 17, 2021

mtei commented Feb 18, 2021

mtei commented Feb 18, 2021

drashna commented Feb 19, 2021

drashna commented Feb 19, 2021

tzarc commented Feb 19, 2021

tzarc commented Feb 19, 2021

tzarc commented Feb 19, 2021

mtei commented Feb 19, 2021

tzarc commented Feb 27, 2021

tzarc commented Feb 28, 2021

tzarc commented Feb 16, 2021 •

edited

Loading

tzarc commented Feb 16, 2021 •

edited

Loading