[Keyboard] Allow custom OLED on Helix Rev3

[drashna]

Description

The rev3 of the Helix contains the oled_user functions but with no way to override them without editing keyboard files. This changes the functions to be weak, so that users can replace them if so desired.

Types of Changes

• Bugfix
• Keyboard (addition or update)

Issues Fixed or Closed by This PR

• convo on discord

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

[mtei]

This change is not necessary. Because if you create your own oled_display.c, everything will be overridden.

Look here.

qmk_firmware/keyboards/helix/rev3_5rows/oled_display.c

Lines 17 to 26 in dd61f77

	/*
	How to Customize
	$ make helix/rev3_5rows:YOUR_KEYMAP:clean
	$ cp keyboards/helix/rev3_5rows/oled_display.c keyboards/helix/rev3_5rows/keymaps/YOUR_KEYMAP
	$ edit keyboards/helix/rev3_5rows/keymaps/YOUR_KEYMAP/oled_display.c
	$ make helix/rev3_5rows:YOUR_KEYMAP
	$ make helix/rev3_5rows:YOUR_KEYMAP:flash
	*/

Keyboards/helix/rev3_5rows/oled_display.c will not compile as in the following example. Instead, keyboards/helix/rev3_5rows/keymaps/newmap/oled_display.c is compiled.

{qmk_firmware} % mkdir keyboards/helix/rev3_5rows/keymaps/newmap   
{qmk_firmware} % cp keyboards/helix/rev3_5rows/keymaps/default/config.h keyboards/helix/rev3_5rows/keymaps/newmap          
{qmk_firmware} % cp keyboards/helix/rev3_5rows/keymaps/default/keymap.c keyboards/helix/rev3_5rows/keymaps/newmap 
{qmk_firmware} % touch keyboards/helix/rev3_5rows/keymaps/newmap/oled_display.c
{qmk_firmware} % make helix/rev3_5rows:newmap

QMK Firmware 0.11.70
WARNING: Some git submodules are out of date or modified.
 Please consider running make git-submodule.

Making helix/rev3_5rows with keymap newmap

avr-gcc (Homebrew AVR GCC 8.4.0_1) 8.4.0
Copyright (C) 2018 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/helix/rev3_5rows/keymaps/newmap/oled_display.c                                 [OK]
Compiling: keyboards/helix/helix.c                                                                  [OK]
Compiling: keyboards/helix/rev3_5rows/rev3_5rows.c                                                  [OK]
Compiling: keyboards/helix/rev3_5rows/keymaps/newmap/keymap.c                                       [OK]
Compiling: quantum/quantum.c                                                                        [OK]
Compiling: quantum/bitwise.c                                                                        [OK]
Compiling: quantum/led.c                                                                            [OK]
Compiling: quantum/keymap_common.c                                                                  [OK]

[drashna]

That's ... less than ideal, IMO. And it's not documented anywhere but in the c file for the oled itself. Which has led to at least one person running into issues when trying to customize their OLED display.

Edit: I think that this change should be okay, as well. Or deed to add that information to the keyboard's readme. I can can do that, if you'd like.

[mtei]

QMK Configurator does not generate any non-keymap data in keymap.c. So for QMK Configurator users, we (MakotoKurauchi and me) have moved the OLED related code from rev3_5rows/keymaps/default/keymap.c to rev3_5rows/oled_display.c. (see af28933, #10297 (review))

Or deed to add that information to the keyboard's readme. I can can do that, if you'd like.

Sure. I think that is the easiest way.

[drashna]

QMK Configurator does not generate any non-keymap data in keymap.c. So for QMK Configurator users, we (MakotoKurauchi and me) have moved the OLED related code from rev3_5rows/keymaps/default/keymap.c to rev3_5rows/oled_display.c. (see af28933, #10297 (review))

That part I definitely understand. And I have done something similar with splitkb's zima keyboard.

However, I feel that the way you have this set up is more complicated and involves additional complexity, when it comes to new users.

Having it there (or even in the keyboard's c file) with the weak attribute is simpler, IMO, and less prone to "why is this erroring out when I compile" issues.

[mtei]

Having it there (or even in the keyboard's c file) with the weak attribute is simpler, IMO, and less prone to "why is this erroring out when I compile" issues.

Is it the keyboard's c file? Not keymap's c file? Could you give me a sample?

For example, I assume that a user would work with the following steps:

1. go to https:/config.qmk.fm, create a keymap and download it by pressing the Export QMK Keymap JSON file button.
2. Create a keyboards/helix/rev3_5rows/keymaps/my_keymap directory and place the downloaded file with the name keymap.json.
3. copy keyboards/helix/rev3_5rows/oled_display.c to keyboards/helix/rev3_5rows/keymaps/my_keymap/oled_display.c.
4. modify keyboards/helix/rev3_5rows/keymaps/my_keymap/oled_display.c.

[drashna]

Is it the keyboard's c file? Not keymap's c file? Could you give me a sample?

Absolutely!
https://github.com/qmk/qmk_firmware/blob/master/keyboards/splitkb/zima/zima.c

I've verified that everything compiles and works correctly when:

• Compiling from source
• Compiling via QMK Configurator
• using my userspace, which includes it's own oled code (though compiles too large if it uses my userspace)

[mtei]

oled_task_user() in splitkb/zima/zima.c completes the OLED code in one function. Therefore, overriding only one function will solve the problem.

However, helix/rev3_Xrows/oled_display.c is made up of multiple functions. It is not recommended to override only the oled_task_user() function.

When oled_task_user() is defined in the user's code, it is desirable to eliminate all the code in helix/rev3_Xrows/oled_display.c.

Therefore, it is recommended to change the keyboard level rules.mk as follows and rename helix/rev3_Xrows/oled_display.c to helix/rev3_Xrows/oled_display_default.c.

--- a/keyboards/helix/rev3_Xrows/rules.mk
+++ b/keyboards/helix/rev3_Xrows/rules.mk
@@ -8,4 +8,4 @@ ENCODER_ENABLE = yes
 DIP_SWITCH_ENABLE = yes
 LTO_ENABLE = yes
 
-SRC += oled_display.c
+LIB_SRC += oled_display_default.c

Below, the same content will be repeated in Japanese for @MakotoKurauchi.

splitkb/zima/zima.c のなかの oled_task_user() は、ひとつの関数のなかで OLED コードが完結しています。したがって、その関数一つだけオーバーライドすれば、解決します。

しかし、helix/rev3_Xrows/oled_display.c は、複数の関数で構成されています。 oled_task_user() 関数一つだけをオーバーライドするのは推奨できません。

ユーザのコードのなかに oled_task_user() が定義されていたときに、helix/rev3_Xrows/oled_display.c のコードをすべて排除するのが望ましいです。

したがって keyboard level の rules.mk を以下のように変更して、helix/rev3_Xrows/oled_display.c を
helix/rev3_Xrows/oled_display_default.c と改名することをおすすめします。

[drashna]

I've changed the sourcing to LIB_SRC, but I'd rather not change the filename, in case somebody has already used it as intended, as it would break their config.

[mtei]

The weak attribute is already useless, so let's remove it.

[mtei]

Incorporating the changes in #12113, I've confirmed that commit 4685926 works as expected.

[drashna]

mtei, I've marked the requests as resolved, for the reasons state here:
#12113 (comment)

[stale]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[noroadsleft]

Slight __attribute__ ((weak)), but LGTM.

[mtei]

About a week ago, I met Makoto Kurauchi to discuss this matter.

As a result, it was agreed to change it to something like #14426.

[drashna]

I saw that, and it's a nice middle ground, I think. Closing this PR in favor of those.