qmk / qmk_firmware

Open-source keyboard firmware for Atmel AVR and Arm USB families
https://qmk.fm
GNU General Public License v2.0
18.23k stars 39.25k forks source link

Tidy up Helix keyboard code to use standard SPLIT_KEYBOARD and OLED_DRIVER_ENABLE features #6690

Closed mcmadhatter closed 2 years ago

mcmadhatter commented 5 years ago

Feature Request Type

Description

The helix keyboard has. a lot of code that predates the move to common split keyboard driver, and a common oled driver. Examples include Serial.c.h I2C.c.h SpitUtil.c.h SSD1306.c.h It would be good to tidy this up and transition over to use the split keyboard feature and oled driver feature.

https://docs.qmk.fm/#/feature_oled_driver?id=oled-driver https://beta.docs.qmk.fm/features/feature_split_keyboard

One other point of discussion:

The other relatively non standard thing that the helix does, is allow for command line override of the build options via a whole load of code in rules.mk e.g.

##    make HELIX=<options> helix:defualt
##    option= oled | back | under | na | ios
##    ex.
##      make HELIX=oled          helix:default
##      make HELIX=oled,back     helix:default
##      make HELIX=oled,under    helix:default
##      make HELIX=oled,back,na  helix:default
##      make HELIX=oled,back,ios helix:default
##

Would it be best to remove this ability so as to match the other qmk keyboards, or to extend it so as to allow the setting of HELIX_ROWS. definition as well, which might make it slightly more useful?

drashna commented 5 years ago

Honestly, this is something you'd want to take up with @mtei, really, as the Helix is his keyboard.

For the additional option, that's actually pretty common, but not really implemented on the keyboard level often.

As for what you describe, that would require a not insignificant change to how the makefiles are designed, or a reversion to how things used to be done. Additionally, parsing the options like that are ... more complicated, especially with make.

mtei commented 5 years ago

The owner of the Helix keyboard is @MakotoKurauchi. I'm just one of the contributors.

However, it is also true that I have the most contributions to the Helix keyboard. And I want to change the Helix keyboard code to use split_common. However, I think split_common is not functional enough to use with the Helix key board.

I've been busy for months and haven't worked on the Helix keyboard. I will resume working when I have time.

mcmadhatter commented 5 years ago

I’ve done the work on my fork, and been using it for a couple of weeks with the common split and oled drivers (it is my daily keyboard at work) Everything works ok on both halves (oled’s, rgb backlight animations, the new scrolling oled animations etc)

Edit.. I have only tried the rev2 keymaps though, as i don’t have a rev1 or pico. The only problematic rev2 keymap is yshrsmz , as it references stuff in the user area, that would also need a minor change.

drashna commented 5 years ago

@mtei I'm curious, what is left to switch it to split common?

The main thing that I can think of is switching it to rgb matrix over rgb light. And if that's the case, this is waiting on #5998, correct?
That, and the build.mk type stuff.

mcmadhatter commented 5 years ago

Works with rgblight https://github.com/mcmadhatter/qmk_firmware/tree/helix_update/keyboards/helix

mtei commented 5 years ago

One other point of discussion:

The other relatively non standard thing that the helix does, is allow for command line override of the build options via a whole load of code in rules.mk e.g.

Helix build options are no different from other keyboards. Specify XXXX_ENABLE = no/yes in keymaps/<keymap>/rules.mk.

The following blocks in rules.mk are not the main way to specify Helix options. Removing this block has no negative impact on the build system.

### Helix keyboard 'default' keymap: convenient command line option
##    make HELIX=<options> helix:defualt
........
##
ifneq ($(strip $(HELIX)),)
........
endif

However, command line overrides are useful for testing multi-configuration builds with shell scripts. That is why this block exists.

mtei commented 5 years ago

Two features of Helix's current code are not yet reflected in split_common.

mtei commented 5 years ago

Currently, I am proposing the following method to MakotoKurauchi. If he agrees, I will try to implement it.

## Helix rev1 old code build
$ make helix/rev1:default

## Helix rev2 old code build
$ make helix/rev2:default

## Helix pico old code build
$ make helix/rev2:default

## Helix rev2 split_command based code build
$ make helix/scv2:default

## Helix pico split_command based code build
$ make helix/scpico:default
zvecr commented 2 years ago

This issue has been automatically closed because it has not had any recent activity. If this issue is still valid, re-open the issue and let us know.