Simplify split_common significantly #4772

pelrun · 2019-01-04T05:01:31Z

Description

Time to throw a cat amongst the pigeons and make some changes that might (well probably will) break something.

Split_common no longer has a separate main loop for the slave, it reuses the keyboard_task main loop like ergodox infinity. This eliminates a lot of the code and makes it a bit easier to reason about. It also means that the slave will behave slightly differently depending on what flags are enabled - many of the options lie outside the is_keyboard_master gate in keyboard_task, and I expect that will have to change as they're tested.

The master/slave transport stuff has also been slighly refactored - instead of i2c_* and serial_* functions and a bunch of ifdefs to change the calling code, it's all been renamed generically, so there's no transport-specific ifdefs in the guts of the matrix code.

Types of changes

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document. (https://docs.qmk.fm/#/contributing)
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).

Both master and slave run the standard keyboard_task main loop now.

Simplify some of the preprocessor mess by using common function names.

tmk_core/common/keyboard.h

drashna · 2019-01-04T05:54:11Z

Also, the changes are causing this issue:
https://travis-ci.org/qmk/qmk_firmware/builds/475165183#L1146

And this:
https://travis-ci.org/qmk/qmk_firmware/builds/475165183#L1247-L1254

pelrun · 2019-01-04T06:31:04Z

Yes, I've changed the semantics significantly and wanted to highlight which existing builds using CUSTOM_MATRIX need to be updated so we can discuss how best to do it.

Looks like miniaxe has a raw pin-per-key scan, that can easily move into split_common. @ka2hiro

And handwired/xealous appears to have an old version of the matrix code, and can probably be converted to just use the standard implementation? @alex-ong

alex-ong · 2019-01-04T08:33:48Z

with regards to my matrix code - it is indeed an older version that i've tweaked a whole bunch to make it faster.
The idea was once my debounce_api stuff got merged (lol it's been in there since september last year), there would be debounce_api_split so scanning/debouncing could be converted to standard implementation too.

For boards with more pins than sense--sorry, switches.

drashna · 2019-01-06T00:58:50Z

Gllad I tested this out fiirst.....
thheeere is sommething definiteely wroonng with thee implemmenntatioin heere. As illustrated by mmy horriblle writtiing..

IIt's not fuunctioioning correctlllyy. IIt's innteerruping keeystrokes on nthe slavee half, whhich cauuses them too be dououble seent, or more.

Nnoote, I usee colemak, annd all of theese errors are onn the slave half.

Edit:
That said, this affects the slave half only. If the slave half is using code before this change, both work just fine. So, it appears to be an issue with sending, I think.

pelrun · 2019-01-06T06:17:46Z

There shouldn't be any difference in the sending code (it only moved about a bit), but the main loop is completely different. My initial guess is that some stuff is being run on the slave that should only happen on master. Matrix_scan() is calling matrix_scan_quantum() for both sides, which is probably completely wrong, and as mentioned earlier there's a bunch of options in keyboard_task that are outside the is_keyboard_master check.

Lets see what happens if I revert the matrix_scan_quantum change and reinstate the matrix_scan_slave_user mechanism instead.

drashna · 2019-01-06T22:24:53Z

Testing out the changes again, and this appears to be a lot better. The issue has gone away.

So that's a plus! :)

drashna · 2019-01-07T20:51:27Z

Well, I don't like that the Xealous keyboard is broken because of these changes. But I'm not sure there is a simple way to fix it (other than turning split common off, copying the files over, and just using the old code....

But that may be fine, honestly. Though, I don't have any i2c based boards to test on, either. Just serial.

@nooges @That-Canadian and @mtei, do these changes look good to you?

pelrun · 2019-01-07T22:17:33Z

I'm working towards getting xealous going, which is why I've just refactored the debounce stuff a bit.

Can now be replaced with custom versions per keyboard using CUSTOM_TRANSPORT = yes and CUSTOM_DEBOUNCE = yes

pelrun · 2019-01-13T04:46:54Z

This should complete the split_common changes, although I still need to convert xealous to use it.

common_features.mk

pelrun · 2019-01-14T13:01:05Z

Documentation is added, and I've merged it with master.

(sorry, I had to revert that recent change to CUSTOM_MATRIX, it directly conflicted and really wasn't better the other way...)

mtei · 2019-01-14T20:45:06Z

@pelrun
The I2C interface is not for master-slave communication only. On the other hand, split_common/serial.c was created for master-slave communication only. Therefore it is possible to use master-slave communication with split_common/serial.c and use the I2C interface at the same time.

For example, drivers/avr/ssd1306.c uses the I2C interface to communicate with the OLED display and requires split_common/i2c.c (or other i2c.c) to operate the I2C interface.

But it seems that your change to common_features.mk assumes the I2C interface for master-slave communication only.

pelrun · 2019-01-15T00:11:09Z

SPLIT_TRANSPORT is specifically intended to exclude files when they would actively cause a problem (e.g. on ARM where neither is compatible.) So it's up to the keyboard to not explicitly exclude a feature it actually requires. If it's unset, then it behaves just like the original build and includes both drivers (so all existing split keyboards build without modification - the travis CI pass should be enough to prove that.)

Anyway, other code shouldn't be using split_common/i2c.c - how would they work on a non-split keyboard that doesn't include that file at all?

drashna · 2019-01-15T06:40:36Z

ideally, we should be using /driver/(arch)/i2c_(master|slave).c rather than a localized version, as we do now.

mtei · 2019-01-15T08:37:41Z

ideally, we should be using /driver/(arch)/i2c_(master|slave).c rather than a localized version, as we do now.

Basically I agree. But can we do it right now? I think that it is better to gradually improve.

pelrun · 2019-01-15T09:37:47Z

It's trivially easy to update the i2c code to use the global driver, but this is already a hefty PR and it'll never land if I keep adding things to it.

mtei · 2019-01-15T09:52:03Z

@pelrun

I do not know why SPLIT_TRANSPORT is necessary. QUANTUM_LIB_SRC works well, I think that it is not necessary to replace with SPLIT_TRANSPORT.

If it's unset, then it behaves just like the original build and includes both drivers.

It behaves not like the original build.

mtei · 2019-01-15T10:03:10Z

Example:
please look the firmware size.

On branch review/Simplify-split_common-significantly

% make lets_split/rev2:default
QMK Firmware 0.6.233
Making lets_split/rev2 with keymap default

avr-gcc (GCC) 7.3.0
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Compiling: keyboards/lets_split/lets_split.c                                           
.....
Compiling: quantum/split_common/transport.c
Compiling: quantum/split_common/i2c.c
Compiling: quantum/split_common/serial.c
......
Checking file size of lets_split_rev2_default.hex
 * The firmware size is fine - 19436/28672 (9236 bytes free)

On branch review/Simplify-split_common-significantly

% make SPLIT_TRANSPORT=serial lets_split/rev2:default
QMK Firmware 0.6.233
Making lets_split/rev2 with keymap default

avr-gcc (GCC) 7.3.0
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Compiling: keyboards/lets_split/lets_split.c
......
Compiling: quantum/split_common/transport.c
Compiling: quantum/split_common/serial.c
.....
Checking file size of lets_split_rev2_default.hex
 * The firmware size is fine - 19212/28672 (9460 bytes free)

The current common_features.mk of the master branch uses QUANTUM_LIB_SRC. So I tried using QUANTUM_LIB_SRC by changing common_features.mk on your branch.

On branch review/Simplify-split_common-significantly
	modified:   common_features.mk

diff --git a/common_features.mk b/common_features.mk
index 4af9ec63a..5b835d96d 100644
--- a/common_features.mk
+++ b/common_features.mk
@@ -277,9 +277,9 @@ ifeq ($(strip $(SPLIT_KEYBOARD)), yes)
 
     # Determine which (if any) transport files are required
     ifndef SPLIT_TRANSPORT
-        QUANTUM_SRC += $(QUANTUM_DIR)/split_common/transport.c \
-                       $(QUANTUM_DIR)/split_common/i2c.c \
-                       $(QUANTUM_DIR)/split_common/serial.c
+        QUANTUM_SRC += $(QUANTUM_DIR)/split_common/transport.c
+        QUANTUM_LIB_SRC += $(QUANTUM_DIR)/split_common/i2c.c \
+                           $(QUANTUM_DIR)/split_common/serial.c
     else
         ifeq ($(strip $(SPLIT_TRANSPORT)), serial)
             QUANTUM_SRC += $(QUANTUM_DIR)/split_common/transport.c

On branch review/Simplify-split_common-significantly
	modified:   common_features.mk

% make lets_split/rev2:default
QMK Firmware 0.6.233
Making lets_split/rev2 with keymap default

avr-gcc (GCC) 7.3.0
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Compiling: keyboards/lets_split/lets_split.c  
.......
Compiling: quantum/split_common/transport.c
Compiling: quantum/split_common/i2c.c
Archiving: .build/obj_lets_split_rev2_default/quantum/split_common/i2c.o
Compiling: quantum/split_common/serial.c
Archiving: .build/obj_lets_split_rev2_default/quantum/split_common/serial.o
.......
Checking file size of lets_split_rev2_default.hex
 * The firmware size is fine - 19212/28672 (9460 bytes free)

Summary:

% make lets_split/rev2:default
 * The firmware size is fine - 19436/28672 (9236 bytes free)
% make SPLIT_TRANSPORT=serial lets_split/rev2:default
 * The firmware size is fine - 19212/28672 (9460 bytes free)
% make lets_split/rev2:default (using QUANTUM_LIB_SRC)
 * The firmware size is fine - 19212/28672 (9460 bytes free)

pelrun · 2019-01-15T10:50:57Z

I've fixed the problem with QUANTUM_LIB_SRC, now that I understand how it's affecting the linking.

It doesn't remove the need for SPLIT_TRANSPORT, which now is like CUSTOM_MATRIX and only allows disabling the current comms code right now, but will select other standard implementations when they're available.

common_features.mk

mtei · 2019-01-15T19:45:34Z

The following functions in quantum/matrix.c and quantum/split_common/matrix.c have the inline keyword.

uint8_t matrix_rows(void)
uint8_t matrix_cols(void)
bool matrix_is_on(uint8_t row, uint8_t col)
matrix_row_t matrix_get_row(uint8_t row)

I think that this 'inline` keyword should be remove.

see: https://gcc.gnu.org/onlinedocs/gcc/Inline.html

mtei · 2019-01-15T21:40:34Z

Indentation of matrix.c:matrix_scan() is somewhat strange.

mtei · 2019-01-15T21:53:22Z

It is better to copy the code of uint8_t matrix_key_count (void) and void matrix_print (void) from quantum/matrix.c.

drashna · 2019-01-15T22:29:46Z

I've fixed the problem with QUANTUM_LIB_SRC, now that I understand how it's affecting the linking.

It may be worth adding a comment on those lines, so that it's clearer why they're handled this way.

drashna · 2019-01-16T01:41:56Z

Is this ready to go, as is?

It looks like it works fine, as I'm using it locally, without issue.

pelrun · 2019-01-16T02:30:24Z

There are no code changes left, just those minor niggling formatting/comment issues, which can easily go into the next PR.

pelrun · 2019-01-16T12:15:29Z

...or they can go into this one because it hasn't been merged yet 💃

mtei · 2019-01-17T12:56:08Z

I tried this split_common PR codes with Patched Helix(i2c with OLED). I only used it for a short time, but it seemed to be working well.

* upstream/master: (390 commits) [Keyboard] handwired/ortho60 Configurator update, readme update, and rules tidy (qmk#4877) Fix Encoder documentation (qmk#4861) TKC1800 refactor and Configurator visual fixes (qmk#4870) Fixed the build break of helix/rev1:OLED_sample caused by PR qmk#4462. (qmk#4874) Add cursor keys to top layer (qmk#4876) [Keyboard] Fix bootloader size for v60_type_r (qmk#4873) Flip definitions of macOS brightness alias Grammatical fixes for GPIO Control doc (qmk#4869) handwired/ortho5x13: layout macro refactor Add new brightness aliases to keycodes_basic.md [Keyboard] update VENDOR_ID, PRODUCT_ID, and DEVICE_VER for Duck boards (qmk#4612) Add Planck rev1 and rev2, clean up rev3-5 config.h Allows Terminal to use ModTap/LayerTap keys (qmk#4586) Defined IS_(HOST_)LED_ON/OFF() and improved LED documentation (qmk#4853) MacOS Brightness Alias (qmk#4836) [Keymap] added custom led effect keymap.c (qmk#4856) Simplify split_common Code significantly (qmk#4772) Add documentation and fix formating (qmk#4860) [Keymap] Adding bdk keymap for ergobox_ez (qmk#4850) 40percent.club Luddite: Configurator update (qmk#4859) ...

* Eliminate separate slave loop Both master and slave run the standard keyboard_task main loop now. * Refactor i2c/serial specific code Simplify some of the preprocessor mess by using common function names. * Fix missing #endif * Move direct pin mapping support from miniaxe to split_common For boards with more pins than sense--sorry, switches. * Reordering and reformatting only * Don't run matrix_scan_quantum on slave side * Clean up the offset/slaveOffset calculations * Cut undebounced matrix size in half * Refactor debouncing * Minor fixups * Split split_common transport and debounce code into their own files Can now be replaced with custom versions per keyboard using CUSTOM_TRANSPORT = yes and CUSTOM_DEBOUNCE = yes * Refactor debounce for non-split keyboards too * Update handwired/xealous to build using new split_common * Fix debounce breaking basic test * Dodgy method to allow a split kb to only include one of i2c/serial SPLIT_TRANSPORT = serial or SPLIT_TRANSPORT = i2c will include only that driver code in the binary. SPLIT_TRANSPORT = custom (or anything else) will include neither, the keyboard must supply it's own code if SPLIT_TRANSPORT is not defined then the original behaviour (include both avr i2c and serial code) is maintained. This could be better but it would require explicitly updating all the existing split keyboards. * Enable LTO to get lets_split/sockets under the line * Add docs for SPLIT_TRANSPORT, CUSTOM_MATRIX, CUSTOM_DEBOUNCE * Remove avr-specific sei() from split matrix_setup Not needed now that slave doesn't have a separate main loop. Both sides (on avr) call sei() in lufa's main() after exiting keyboard_setup(). * Fix QUANTUM_LIB_SRC references and simplify SPLIT_TRANSPORT. * Add comments and fix formatting.

pelrun added 3 commits January 4, 2019 15:47

Eliminate separate slave loop

6bf2430

Both master and slave run the standard keyboard_task main loop now.

Refactor i2c/serial specific code

609668c

Simplify some of the preprocessor mess by using common function names.

Fix missing #endif

206915e

drashna reviewed Jan 4, 2019

View reviewed changes

tmk_core/common/keyboard.h Show resolved Hide resolved

drashna added enhancement core labels Jan 4, 2019

pelrun added 2 commits January 4, 2019 21:21

Move direct pin mapping support from miniaxe to split_common

34c100d

For boards with more pins than sense--sorry, switches.

Reordering and reformatting only

f8d234d

Don't run matrix_scan_quantum on slave side

afa3951

pelrun added 4 commits January 8, 2019 00:18

Clean up the offset/slaveOffset calculations

9013ea2

Cut undebounced matrix size in half

882be6f

Refactor debouncing

71028fc

Minor fixups

5d584dc

pelrun force-pushed the split_common_arm branch from a37c4b2 to 5d584dc Compare January 7, 2019 14:43

drashna added the needs testing label Jan 7, 2019

Split split_common transport and debounce code into their own files

279e24d

Can now be replaced with custom versions per keyboard using CUSTOM_TRANSPORT = yes and CUSTOM_DEBOUNCE = yes

pelrun force-pushed the split_common_arm branch from d90a74e to 279e24d Compare January 13, 2019 04:45

drashna reviewed Jan 13, 2019

View reviewed changes

common_features.mk Outdated Show resolved Hide resolved

drashna reviewed Jan 13, 2019

View reviewed changes

common_features.mk Outdated Show resolved Hide resolved

pelrun added 2 commits January 13, 2019 15:59

Refactor debounce for non-split keyboards too

686f387

Update handwired/xealous to build using new split_common

7f7536a

Fix QUANTUM_LIB_SRC references and simplify SPLIT_TRANSPORT.

332ef9a

drashna reviewed Jan 15, 2019

View reviewed changes

common_features.mk Show resolved Hide resolved

Add comments and fix formatting.

9db5a3f

pelrun force-pushed the split_common_arm branch from d7ec226 to 9db5a3f Compare January 17, 2019 01:09

drashna removed the needs testing label Jan 17, 2019

drashna merged commit 28929ad into qmk:master Jan 17, 2019

pelrun deleted the split_common_arm branch January 18, 2019 12:42

pelrun mentioned this pull request Jan 22, 2019

Debounce refactor / API #3720

Merged

pelrun mentioned this pull request Feb 26, 2019

Clean up debounce a bit #5255

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify split_common significantly #4772

Simplify split_common significantly #4772

pelrun commented Jan 4, 2019 •

edited

Loading

drashna commented Jan 4, 2019

pelrun commented Jan 4, 2019

alex-ong commented Jan 4, 2019 •

edited

Loading

drashna commented Jan 6, 2019 •

edited

Loading

pelrun commented Jan 6, 2019

drashna commented Jan 6, 2019

drashna commented Jan 7, 2019 •

edited

Loading

pelrun commented Jan 7, 2019

pelrun commented Jan 13, 2019

pelrun commented Jan 14, 2019

mtei commented Jan 14, 2019

pelrun commented Jan 15, 2019 •

edited

Loading

drashna commented Jan 15, 2019

mtei commented Jan 15, 2019

pelrun commented Jan 15, 2019

mtei commented Jan 15, 2019

mtei commented Jan 15, 2019 •

edited

Loading

pelrun commented Jan 15, 2019

mtei commented Jan 15, 2019

mtei commented Jan 15, 2019

mtei commented Jan 15, 2019 •

edited

Loading

drashna commented Jan 15, 2019

drashna commented Jan 16, 2019

pelrun commented Jan 16, 2019

pelrun commented Jan 16, 2019

mtei commented Jan 17, 2019 •

edited

Loading

Simplify split_common significantly #4772

Simplify split_common significantly #4772

Conversation

pelrun commented Jan 4, 2019 • edited Loading

Description

Types of changes

Checklist:

drashna commented Jan 4, 2019

pelrun commented Jan 4, 2019

alex-ong commented Jan 4, 2019 • edited Loading

drashna commented Jan 6, 2019 • edited Loading

pelrun commented Jan 6, 2019

drashna commented Jan 6, 2019

drashna commented Jan 7, 2019 • edited Loading

pelrun commented Jan 7, 2019

pelrun commented Jan 13, 2019

pelrun commented Jan 14, 2019

mtei commented Jan 14, 2019

pelrun commented Jan 15, 2019 • edited Loading

drashna commented Jan 15, 2019

mtei commented Jan 15, 2019

pelrun commented Jan 15, 2019

mtei commented Jan 15, 2019

mtei commented Jan 15, 2019 • edited Loading

pelrun commented Jan 15, 2019

mtei commented Jan 15, 2019

mtei commented Jan 15, 2019

mtei commented Jan 15, 2019 • edited Loading

drashna commented Jan 15, 2019

drashna commented Jan 16, 2019

pelrun commented Jan 16, 2019

pelrun commented Jan 16, 2019

mtei commented Jan 17, 2019 • edited Loading

pelrun commented Jan 4, 2019 •

edited

Loading

alex-ong commented Jan 4, 2019 •

edited

Loading

drashna commented Jan 6, 2019 •

edited

Loading

drashna commented Jan 7, 2019 •

edited

Loading

pelrun commented Jan 15, 2019 •

edited

Loading

mtei commented Jan 15, 2019 •

edited

Loading

mtei commented Jan 15, 2019 •

edited

Loading

mtei commented Jan 17, 2019 •

edited

Loading