Re-Unify Split Keyboard Code and add ARM support

drashna commented 6 years ago

Specifically, @mtei and @MakotoKurauchi has done some awesome work to expand the functionality of the serial code for some of the split keyboards.

However, this has happened independently and outside of the Split Common code. Meaning that the improvements made are not improved the split code, and have cause it to diverge further, which was the entire point of the Split Common code: to remove the heavily fragmentation that was happening, and make the code easier to maintain for all (most) split keyboards.

Additionally, the Orthodox and BFO9000 are using customized matrix code to allow for more than 8 columns. This should be ported over to the Split Common code, as well.

This isn't to attack or berate them for their work. It's pointing out a big issue that I see, and has been bothering me.

Ideally, these two sets of files need to be merged together, ind a way that works for everyone.

[x] Configurable Serial Pin
[x] Support both I2C and Serial at the same time that works with the configurator
[x] Soft Serial
[X] Support more than 8 columns/rows
- [x] Serial
- [X] i2c
[ ] ~~Expand i2c code to allow for chained devices (so that you can have the board using i2c for communication and controlling the displays on the helix)~~
[x] Convert I2C based communication to use /drivers/*/i2c_master.c (This provides better ARM support, as there is an AVR and ARM version of i2c_master)
- [ ] Add support for i2c_slave on ARM (needs to be created)
[x] Update Serial communication to support ARM
[x] Migrate Helix, CRKBD, or other similiar boards to Split Common, to verify that it's all up to date
- [x] Update CRKBD to use newer OLED code found in Helix keyboard code

Also tagging @That-Canadian and @nooges about this, in the hopes that they have some input/can help hout here.

Lenbok commented 6 years ago

In most cases converting keyboards over to use the split common code is fairly straight forward, but just needs someone that actually has one of the boards to do a final test.

Perhaps when someone submits any PR targetting one of these boards, ask them to first convert the board to split common (since presumably they have one of the boards to test)?

drashna commented 6 years ago

Testing is definitely ideal. However, some of the code for the helix code has diverged greatly from the split code, in general (eg, that of the lets Split). So this isn't just a small project. And getting it to work with bot sets (basically) will absolutely need testing.

And the other factor is how to handle the configurator.

mtei commented 6 years ago

helix/serial.c has enough backward compatibility. Please look at the sample. #4293

The problem is that split_common/serial.c is not configurable. I think that serial.c should have been made configurable when quantum/split_common was created.

drashna commented 6 years ago

@mtei okay, good to know.

And absolutely agree with you.

Unfortunately, I'm definitely still learning (though, that's a good thing, too, as there is always more to learn!), and didn't catch that, at the time.

To update the code would be more of a "merge" than a copy job.

Additionally, one of the issues that I foresee here is the "use serial", "use i2c" paradigm. Right now, the split common code is configured in such a way that it's really one or the other, but I know that won't work well for how the Helix (and similar boards) work.

The other issue with this paradigm is that it has issues on the Configurator. Basically, anything that relies on the config.h or rules.mk of the keymap folder is not usable on the Configurator, and may cause the build process to fail.

Also, I'll probably look at the code you've linked, and see if I can hack something together.

Because I really would love to have a properly unified split common code, that works for everyone.

mtei commented 6 years ago

helix-serial.c supports the following APIs compatible with let's split. https://github.com/qmk/qmk_firmware/blob/e18eab7a81c51015c8bd81931f90cf1dc808ac78/keyboards/lets_split/serial.h-old#L15-L25

helix-serial.c also supports some new APIs. https://github.com/qmk/qmk_firmware/blob/e18eab7a81c51015c8bd81931f90cf1dc808ac78/keyboards/lets_split/serial.h#L44-L69

The let's split compatible APIs is implemented as a wrapper function in the following part on helix-serial.c. https://github.com/qmk/qmk_firmware/blob/e18eab7a81c51015c8bd81931f90cf1dc808ac78/keyboards/lets_split/serial.c#L73-L116

I think the wrapper functions on helix-serial.c should be moved to matrix.c (or a new file). By so doing, serial.c can be immutable without being affected by changes in the caller's source (matrix.c or other source files). Also, it is possible to exclude SERIAL_SLAVE_BUFFER_LENGTH and SERIAL_ MASTER_BUFFER_LENGTH from config.h, and the setting is simplified.

mtei commented 6 years ago

In #4379, I tried solving the following problem.

Configurable Serial Pin
The data structure of the master-slave communication was made more freely.
Support both I2C and Serial at the same time.

Try compile in this branch. Like this.

$ make RULES_VERBOSE=yes OLED_ENABLE=yes SPLIT_COMMUNICATION=serial lets_split:default

mtei commented 5 years ago

draft: new configuration about use i2c, serial

When communication between master and slave is serial

In keyboards/KEYBOARD/rules.mk

SPLIT_KEYBOARD = yes
SPLIT_COMMUNICATION = serial
OLED_ENABLE = yes # or no
# if need   SOME_DEVICE_ENABLE = yes or no it's use I2C

In keyboards/KEYBOARD/config.h

#define SOFT_SERIAL_PIN D0 // or D1, D2, D3, E6

When communication between master and slave is I2C

In keyboards/KEYBOARD/rules.mk

SPLIT_KEYBOARD = yes
SPLIT_COMMUNICATION = i2c
OLED_ENABLE = yes # or no
# if need   SOME_DEVICE_ENABLE = yes or no it's use I2C

In keyboards/KEYBOARD/config.h

/* nothing */

qmk_firmware/common_features.mk implementation

ifeq ($(strip $(SPLIT_KEYBOARD)), yes)
  #
  # SPLIT_COMMUNICATION and OLED_ENABLE settings convert to C lang Macros
  #
  USE_I2C := no
  SPLIT_SRC := $(QUANTUM_DIR)/split_common/split_flags.c
  SPLIT_SRC +=  $(QUANTUM_DIR)/split_common/split_util.c 

  ifeq ($(strip $(SPLIT_COMMUNICATION)), serial)
    # add serial.c
    SPLIT_SRC += $(QUANTUM_DIR)/split_common/serial.c
    # set SPLIT_COMMUNICATION_SERIAL for matrix.c and split_util.c
    OPT_DEFS += -DSPLIT_COMMUNICATION_SERIAL
  else
    ifeq ($(strip $(SPLIT_COMMUNICATION)), i2c)
      # set SPLIT_COMMUNICATION_I2C for matrix.c and split_util.c
      OPT_DEFS += -DSPLIT_COMMUNICATION_I2C
      USE_I2C = yes
    else
      $(error invalid SPLIT_COMMUNICATION value)
    endif
  endif

  ifeq ($(strip $(OLED_ENABLE)), yes)
    SPLIT_SRC += ssd1306.c
    # set SSD1306OLED for split_util.c
    OPT_DEFS += -DSSD1306OLED
    USE_I2C = yes
  endif

  # is there any module using I2C?
  ifeq ($(strip $(USE_I2C)), yes)
    # add i2c.c
    SPLIT_SRC += $(QUANTUM_DIR)/split_common/i2c.c
    # set USE_AVR_ATmega32U4_I2C for seria.c(for conflict check)
    OPT_DEFS += -DUSE_AVR_ATmega32U4_I2C
  endif

  ifdef SPLIT_RULES_VERBOSE
    $(info *** split keyboarad configuration ***)
    $(info  SRC += $(SPLIT_SRC))
    $(info  USE_I2C = $(USE_I2C))
    $(info  SPLIT_COMMUNICATION = $(SPLIT_COMMUNICATION))
    $(info  OPT_DEFS = $(OPT_DEFS))
  endif

  QUANTUM_SRC += $(SPLIT_SRC)
endif

drashna commented 5 years ago

I don't think all of the feature code is appropriate here. A lot of this should be relegated to defines, and not feature rules. But that's more of semantics, though.

Additionally, for the boards using i2c for something other than split communication, it may be better (long term) to use the main i2c driver, and not the split code (though, the split code should see about using the main driver too, eventually). You can see the i2c drivers here: https://github.com/qmk/qmk_firmware/tree/master/drivers/avr

This would allow for much more advanced configurations, and remove some of the "mess" that exists right now.

Additionally, in theory, for the boards like the helix, because i2c uses addresses, it theory you could use i2c for both split communication and for communicating with things like the OLED displays (and both of them, actually). However, I do understand that this is much more complicated to implement, and cleanly.

mtei commented 5 years ago

I think that it is best that the objects of drivers is contained in the library file as follows.

In keyboards/KEYBOARD/config.h

#define SPLIT_COMMUNICATION_SERIAL
#define SOFT_SERIAL_PIN D0 // or D1, D2, D3, E6
// or #define SPLIT_COMMUNICATION_I2C

In i2c.c

// #ifdef USE_I2C   /* <-- remove this ifdef */
.....
// #endif

In serial.c

// #ifndef USE_I2C   /* <-- remove this ifndef */
.....
// #endif

In matrix.c and split_util.c

#if defined(SPLIT_COMMUNICATION_I2C)
 // using I2C code
#elif defined(SPLIT_COMMUNICATION_SERIAL)
 // using serial code
#endif

compile and link sequence

$ avr-gcc -c split_common/serial.c
$ avr-gcc -c split_common/i2c.c  # or drviers/avr/i2c_master.c ??
$ avr-ar cvr libdrivers.a serial.o i2c.o
...
$ avr-gcc -c <each .c files>
...
$ avr-gcc -o keyboard.elf $(OBJS) -ldrivers # automatically select serial.o, i2c.o

But I didn't know how to add library generation rules to the Qmk_firmware makefile. As the second best method, I thought the above qmk_firmware/common_features.mk idea.

LouWii commented 5 years ago

Sorry to show up in this thread, I have kind of a related question.

I'd like to create a QMK firmware build for a split keyboard I created, I was looking for some help/starting point and ended up here.

As I understand, there's a common base code for split boards but you seem to be working on it. Should I wait that the changes are merged before starting any work?

I was also looking at the existing firmwares for split boards, trying to find one that has a good code base that I could get some inspiration from. Do you know one that would be a good starting point?

Thanks

mtei commented 5 years ago

I posted #4522. This is a concrete solution I thought about comments https://github.com/qmk/qmk_firmware/issues/4254#issuecomment-439307124 and https://github.com/qmk/qmk_firmware/issues/4254#issuecomment-439717577.

drashna commented 5 years ago

@LouWii Depending on what you're doing, you can look at the iris keyboard, or helix keyboard for how they're configured.

For the most part, you just enable SPLIT_KEYBOARD = yes in the keyboard's rules.mk file, and there are some config.h settings.

@mtei Well, one thing that has bothered me, is that ... in theory, there is no reason that the Helix keyboard couldn't use i2c for the communication. And if the OLED screens have unique addresses, you could include them in the chain, and be able to control both from either side.
Though, this is assuming a lot. Unfortunately.

Additionally, there is the i2c_master.c "driver" available. Using that may be a good idea for the split keyboards in general. But may be much better for the OLED screen support, in general. As this would cause less issues with the split stuff.

rgb matrices, and qwiic devices use i2c, and use the /drivers/*/i2c_master.c file.

And thinking about it, maybe we should treat the SPLIT_KEYBOARD more like an array, than a boolean, so we could have SPLIT_KEYBOARD set to "i2c", or "serial" (with "yes" defaulting to serial, or maybe a "both" option.

mtei commented 5 years ago

@drashna

Well, one thing that has bothered me, is that ... in theory, there is no reason that the Helix keyboard couldn't use i2c for the communication.

From the results below, I think that it is better to communicate master and slave of Helix keyboard in serial rather than I2C.

I tried changing and testing the split_common code by connecting the Helix keyboard to the breadboard.

https://github.com/mtei/qmk_firmware/tree/Test_of_quantum_split_common_with_helix_keyboard/keyboards/handwired/pdhelix

As a result, it was conditionally possible to communicate between master and slave on Helix keyboard with I2C. If you do not attach the OLED module or if only one is attached, both can communicate with I2C.

When no OLED module is attached https://github.com/mtei/qmk_firmware/tree/Test_of_quantum_split_common_with_helix_keyboard/keyboards/handwired/pdhelix/i2c
When only one OLED module is attached https://github.com/mtei/qmk_firmware/tree/Test_of_quantum_split_common_with_helix_keyboard/keyboards/handwired/pdhelix/i2c_1oled

However, it is not possible to have two OLED modules.

In addition, it was possible to perform serial communication using PD2 between master and slave of Helix keyboard and to attach two OLEDs.

https://github.com/mtei/qmk_firmware/tree/Test_of_quantum_split_common_with_helix_keyboard/keyboards/handwired/pdhelix/pd2_2oled

mtei commented 5 years ago

Additionally, there is the i2c_master.c "driver" available. Using that may be a good idea for the split keyboards in general. But may be much better for the OLED screen support, in general. As this would cause less issues with the split stuff.

rgb matrices, and qwiic devices use i2c, and use the /drivers/*/i2c_master.c file.

Currently, compilation of i2c.c and serial.c under quantum/split_common/ is exclusively switched depending on whether there is a USE_I2C macro or not.

I think it's a good idea to change to use a common driver rather than using its own i2c.c under quantum/split_common/. If so, we should stop using the USE_I2C macro as a compilation switch.

PullRequest #4522 provides a method to compile both i2c.c and serial.c and link only what is needed at link time.

drashna commented 5 years ago

@ the OLED screens.

Okay, I had a feeling that this was the case. And I'm sorry to hear that it was. It really sucks. Because it would be pretty awesome to have both working, and working over i2c exclusively.

Also, this is unrelated, but the OLED screen code in the keymap... you could use biton32(layer_state) to handle the layer codes, that way, you don't have to use the actual mask.
In fact, I'm doing that on the crkbd, and verify that it works well: https://github.com/qmk/qmk_firmware/blob/master/keyboards/crkbd/keymaps/drashna/keymap.c#L185-L215

mtei commented 5 years ago

Also, this is unrelated, but the OLED screen code in the keymap... you could use biton32(layer_state) to handle the layer codes, that way, you don't have to use the actual mask. In fact, I'm doing that on the crkbd, and verify that it works well:

Thank you for telling me biton32(). It was good to know.

That code was copied from Helix keymap 'five_rows'. And in 'five_rows', I wanted to display all the active layers. Because 'five_rows' has a few layers that are almost transparent. So biton32() can not be used in this case.

drashna commented 5 years ago

Ah, yeah, if you wanted to display all of the layers, then that wouldn't work. But I'm glad that I mentioned it, regardless.

mtei commented 5 years ago

I am currently working on bringing helix-serial.c to quantum/split_common.

It is known that keyboard MiniAxe can not be compiled.

https://github.com/mtei/qmk_firmware/commits/replace_serial_c_of_quantum_split_common

mtei commented 5 years ago

It is almost completed, and I also confirm that it works.

However, miniaxe has been remodeled by copying matrix.c from split_common/, so it is stranded from the change of splir_common/matrix.c this time.

drashna commented 5 years ago

That's fine. I'll see about tagging the creator of the board later, so they can see about updating the board.

And again, thank you for all of the work!

That said, with the Proton C release, it may be worth looking into converting/adding support for ARM. Maybe not initially, but at least to keep it in mind.

For reference, these are how to address the pins/ports in ARM: https://github.com/qmk/qmk_firmware/blob/master/docs/internals_gpio_control.md

mtei commented 5 years ago

I contacted @ka2hiro, creator of miniaxe, and I got permission to change keyboards/miniaxe/matrix.c.

BrianCArnold commented 5 years ago

I've got an Iris and a Quefrency, I'm entirely willing to resolder Proton-C's on them and work on testing them.

pelrun commented 5 years ago

Hold off on that for the moment; I'm doing a bunch of work on split_common at the moment for ARM support and it's not there yet.

BrianCArnold commented 5 years ago

Can you link me to your branch? I've been wanting to understand how it works.

Gimly commented 5 years ago

I would like to help adding support for more than 8 columns since I have that need for a keyboard I have built that is a split TKL and has 11 columns (on the right side, 8 on the left).

I've seen some TODOs in the transport.c code that talks about using pack/unpack, is this still the way to go and what are those methods? Do someone have a link explaining them?

pelrun commented 5 years ago

Serial has supported >8 columns since before I started work on this, it was only i2c that was restricted. You can ignore the comment, that's only talking about compressing the matrix data.

Gimly commented 5 years ago

@pelrun I'm using i2c, so is it still restricted to <= 8 columns in i2c, and if so what needs to be done to fix that?

Lenbok commented 5 years ago

@Gimly You can help test #4974 which aims to remove the column restriction for i2c splits.

drashna commented 5 years ago

@Gimly The current issue is that the i2c code in #4974 does not work. I'm not sure why, as the code looks okay.

That PR fixes the column limit, already. But the i2c code needs work and testing.

Once that's done, we can merge it and be significantly closer to ARM support as well.

pelrun commented 5 years ago

The current issue is that the i2c code in #4974 does not work.

It's been fixed for a couple of days now :)

Gimly commented 5 years ago

Great, I'll do some tests tomorrow with #4974 code. I'll update with my findings. Thanks!

Oh, another thing, I've used 2 Teensy for my keyboard and the current code for is_keyboard_master doesn't work for Teensy. I had to add the weak attribute to the method and force it with return true / return false depending on which side I am programming. I wonder if there would be an implementation that would work also for Teensy. I think it's the USB detection that doesn't work exactly the same on the Teensy than on the Arduino Pro Micro.

And while I'm at it, my Keyboard has a non symmetric number of columns, left side has 7 and right side has 11 (it's a split TKL). It would be great if I could have a way to configure a different matrix for each side.

yurikhan commented 5 years ago

+1 for another method of master detection. I am using an Arduino clone (CNT-013) that shorts VUSB to +5V so the usual method will not work.

vitamins_included has a different solution: power on as slave, become master when assigned a USB address, or continue as slave when contacted by a master over I2C or serial link.

XScorpion2 commented 5 years ago

So I did a file based analysis on the i2c.h/c versions in use in qmk and the results can be found here: https://drive.google.com/open?id=1ySZlhbUucAQoQBnpiZ8rYX3LtNFvkdnP There are basically 44 copies, that boil down to 3 unique versions: split common, split common with oled functions, and the Luiz Ribeiro's version. If you open i2c_analysis.txt in that drive folder, it goes into more details.

drashna commented 5 years ago

Updated the main post, with additional information, and ARM stuff

DeepwaterCreations commented 5 years ago

Hey, is there a good way for a random coder such as myself to help out with this issue? I'm itching to get my Proton-C + Viterbi build underway. :)

drashna commented 5 years ago

absolutely.

RIght now, the remaining issues are:

No i2c_slave.c for ARM
Serial is hard coded for AVR.
- It would need to be generalized, or a separate one written for ARM, and the existing one moved to /drivers/avr/
i2c_master for AVR is not properly licensed, and needs to be replaced with a GPL Compliant version.

From there, I think everything is ready for ARM. But we won't know for certain until those are fixed up. ,

@pelrun has been the one spearheading it, but I'm not sure where he is, at it, right now.
@mtei has also been very active with the split keyboard code, and may be able to help.

If you want to contribute and help here, and have the experience, then it may be worth heading to discord and chatting up pelrun. and others. And chatting on the progress.

kyleterry commented 5 years ago

What is the status of this issue?

I've been working on revisions of my own split board called the Spacetime keyboard that's similar to the Corne and the helix as far as the circuit goes. My goal is to not move any of the serial and split code from those keyboards over to the spacetime board and to use split common and the avr i2c code in qmk core.

The keyboard has 7 columns and 4 rows on each half, an oled on each half, 50 rgb led matrix (25 on each half) and uses serial over TRRS on D2. The oleds are attached to scl and sda for i2c and the leds are attached to D3.

A lot of stuff works right away without a lot of configuration. RGB backlight with animations work, although there is some serious power draw issues, so I have to limit the brightness pretty significantly, otherwise the slave half locks up when the controller doesn't have enough power. This might be a hardware design problem I have, but I think it's a known thing that can happen with a lot of leds.

RGB matrix does not work with the core serial split code. the slave half doesn't function right if I try to use the matrix feature. This might be my matrix code, but I would like to confirm if this is a known thing.

The oleds do not work with the core serial split and i2c code. I think this is known as well (it looks like there were a lot of changes made to serial and i2c code out-of-band in the crkbd and helix boards), but I would like to confirm that is a thing as well.

So with all that said, I have a board I can experiment with to get some of these features moved over, I just need to know what are known broken things that aren't being worked on that I can fix. Since my board functions almost exactly like those two boards, I have a clean slate to work with and can work on getting things up and running.

drashna commented 5 years ago

@kyleberry it's mostly there.

But your issue is more with how the split code works in general. #5998 seeks to address that issue, actually. And I've tested it with the Corne using #6001 as well, and can verify it works.

cmpulsip commented 5 years ago

Hello @drashna, I've been following the progress of this issue very closely as I'd like to build a split keyboard using two ARM-based microcontrollers(STM32) one day. I'm new to building a firmware, but would like to get more familiar and help in any way I can. I see that you replied to @DeepwaterCreations above about the current state of this issue. Does that list still stand, or is there another aspect in which I might offer a small contribution?

Thanks.

drashna commented 5 years ago

The list is up to date.

Serial and i2c_slave support need to be added for ARM, at this point.

stale[bot] commented 4 years ago

This issue has been automatically marked as resolved because it has not had activity in the last 90 days. It will be closed in the next 30 days unless it is tagged properly or other activity occurs.

paraqles commented 4 years ago

Anything new about this?

I'm unfortunately not so well versed with C. And especially not with ARM. Otherwise I would like to help. Are there any starting points to get into position to help?

drashna commented 4 years ago

It's mostly there.

There are a few PRs open that are relevant, and it may be worth taking a look at.

Jumping onto discord may be the best best for talking about it

ClaudiaLapalme commented 3 years ago

What's the status on this? Can I help?

tzarc commented 3 years ago

Should be pretty much complete at this point. Will have a look through things this weekend and see if there's anything else outstanding.

drashna commented 3 years ago

Closing this, because, yeah, this is pretty much complete.

Only thing missing is i2c_slave for chibiOS, but that's ... a can of worms.

Also, helix should be ported over

mtei commented 3 years ago

Report about Helix keyboad scan rate

With the improvements made by #11930, the performance of split_common has caught up with the local implementation of Helix (rev2).

before 0.14.0

use split_common	oled	rgblight back	scan rate
no (local)	disable	disable	1631
no (local)	enable	enable	1515
yes	disable	disable	880
yes	enable	enable	829

after 0.14.0

use split_common	oled	rgblight back	scan rate
no (local)	disable	disable	1627
no (local)	enable	enable	1513
yes	disable	disable	1555
yes	enable	enable	1360

So, Helix keymaps that have been switched to using oled_driver.c in the OLED code can be switched to use split_common.

qmk / qmk_firmware