Bastard Keyboards: upstream recent changes

[0xcharly]

Description

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Issues Fixed or Closed by This PR

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

[fauxpark]

bastardkb/charybdis/3x5/v2/splinky/v2 etc are invalid; QMK only supports up to 5 levels of nesting. This has caused some build failures in CI due to the placement of the pointing device CS pin define:

bastardkb/charybdis/3x5/v2/splinky:default
bastardkb/charybdis/3x5/v2/splinky:via
bastardkb/charybdis/3x6/v2/splinky:default
bastardkb/charybdis/3x6/v2/splinky:via
bastardkb/charybdis/4x6/v2/splinky:default
bastardkb/charybdis/4x6/v2/splinky:via

[drashna]

Also, I missed it but in dilemma.c, line 334,

void keyboard_pre_init_user(void) {

This should be the kb version, and then call the user version. (appears to be the only place this sort of issue has occurred).

[0xcharly]

bastardkb/charybdis/3x5/v2/splinky/v2 etc are invalid; QMK only supports up to 5 levels of nesting. This has caused some build failures in CI due to the placement of the pointing device CS pin define

This seems to be an arbitrary limit, and I can't seem to find any reference to it. It also doesn't match the output of the CI builder which shows that bastardkb/charybdis/3x5/v2/splinky/v2:default, bastardkb/charybdis/3x5/v2/splinky/v3:default, and others with similar path built fine, but bastardkb/charybdis/3x5/v2/splinky:default, for example, did not.

According to https://github.com/qmk/qmk_firmware/blob/master/lib/python/qmk/keyboard.py#L101-L108, it seems that QMK's CLI is defining a keyboard folder as a folder that contains a rules.mk file. This explains the CI behavior above, because v2/splinky folders do contain and share a rules.mk file that is shared between their children folders v2/splinky/v2 and v2/splinky/v3. The same thing happens for the Scylla, TBK mini, and Skeletyl boards, where the row pin definition changes based on the Splinky's version, so eg.

qmk compile -c -j 72 -kb bastardkb/scylla/v2/splinky -km default

Compiles, but yields a non-functioning firmware. The issue is actually flagged at compile time by a warning:

quantum/matrix.c:53:32: warning: 'row_pins' defined but not used [-Wunused-const-variable=]
   53 | static SPLIT_MUTABLE_ROW pin_t row_pins[ROWS_PER_HAND] = MATRIX_ROW_PINS;
      |                                ^~~~~~~~

In light of this, I believe that the appropriate course of action is to use DEFAULT_FOLDER in v2/splinky/rules.mk to default to building v2/splinky/v3. An alternative would be to remove v2/splinky/rules.mk and duplicate it into v2/splinky/v2 and v2/splinky/v3, but I'd like to maintain code/config reuse here if possible.

#19093 adds the necessary DEFAULT_FOLDER declarations, which seems to satisfy the CI's automatic keyboard folder discovery mechanism.

I also verified locally that these targets now build fine and with no warnings:

• bastardkb/charybdis/3x5/v2/splinky:default
• bastardkb/charybdis/3x6/v2/splinky:default
• bastardkb/charybdis/4x6/v2/splinky:default
• bastardkb/scylla/v2/splinky:default
• bastardkb/tbkmini/v2/splinky:default
• bastardkb/skeletyl/v2/splinky:default

[0xcharly]

Also, I missed it but in dilemma.c, line 334,

void keyboard_pre_init_user(void) {

This should be the kb version, and then call the user version. (appears to be the only place this sort of issue has occurred).

Addressed in #19091.

[drashna]

This seems to be an arbitrary limit

It is. And the code that arbitrates that is here:

qmk_firmware/builddefs/build_keyboard.mk

Lines 59 to 91 in df5689e

	# Determine which subfolders exist.
	KEYBOARD_FOLDER_PATH_1 := $(KEYBOARD)
	KEYBOARD_FOLDER_PATH_2 := $(patsubst %/,%,$(dir $(KEYBOARD_FOLDER_PATH_1)))
	KEYBOARD_FOLDER_PATH_3 := $(patsubst %/,%,$(dir $(KEYBOARD_FOLDER_PATH_2)))
	KEYBOARD_FOLDER_PATH_4 := $(patsubst %/,%,$(dir $(KEYBOARD_FOLDER_PATH_3)))
	KEYBOARD_FOLDER_PATH_5 := $(patsubst %/,%,$(dir $(KEYBOARD_FOLDER_PATH_4)))
	KEYBOARD_FOLDER_1 := $(notdir $(KEYBOARD_FOLDER_PATH_1))
	KEYBOARD_FOLDER_2 := $(notdir $(KEYBOARD_FOLDER_PATH_2))
	KEYBOARD_FOLDER_3 := $(notdir $(KEYBOARD_FOLDER_PATH_3))
	KEYBOARD_FOLDER_4 := $(notdir $(KEYBOARD_FOLDER_PATH_4))
	KEYBOARD_FOLDER_5 := $(notdir $(KEYBOARD_FOLDER_PATH_5))
	KEYBOARD_PATHS :=
	KEYBOARD_PATH_1 := keyboards/$(KEYBOARD_FOLDER_PATH_1)
	KEYBOARD_PATH_2 := keyboards/$(KEYBOARD_FOLDER_PATH_2)
	KEYBOARD_PATH_3 := keyboards/$(KEYBOARD_FOLDER_PATH_3)
	KEYBOARD_PATH_4 := keyboards/$(KEYBOARD_FOLDER_PATH_4)
	KEYBOARD_PATH_5 := keyboards/$(KEYBOARD_FOLDER_PATH_5)
	ifneq ("$(wildcard $(KEYBOARD_PATH_5)/)","")
	KEYBOARD_PATHS += $(KEYBOARD_PATH_5)
	endif
	ifneq ("$(wildcard $(KEYBOARD_PATH_4)/)","")
	KEYBOARD_PATHS += $(KEYBOARD_PATH_4)
	endif
	ifneq ("$(wildcard $(KEYBOARD_PATH_3)/)","")
	KEYBOARD_PATHS += $(KEYBOARD_PATH_3)
	endif
	ifneq ("$(wildcard $(KEYBOARD_PATH_2)/)","")
	KEYBOARD_PATHS += $(KEYBOARD_PATH_2)
	endif
	ifneq ("$(wildcard $(KEYBOARD_PATH_1)/)","")
	KEYBOARD_PATHS += $(KEYBOARD_PATH_1)
	endif

[0xcharly]

This seems to be an arbitrary limit

It is. And the code that arbitrates that is here:

qmk_firmware/builddefs/build_keyboard.mk

Lines 59 to 91 in df5689e

	# Determine which subfolders exist.
	KEYBOARD_FOLDER_PATH_1 := $(KEYBOARD)
	KEYBOARD_FOLDER_PATH_2 := $(patsubst %/,%,$(dir $(KEYBOARD_FOLDER_PATH_1)))
	KEYBOARD_FOLDER_PATH_3 := $(patsubst %/,%,$(dir $(KEYBOARD_FOLDER_PATH_2)))
	KEYBOARD_FOLDER_PATH_4 := $(patsubst %/,%,$(dir $(KEYBOARD_FOLDER_PATH_3)))
	KEYBOARD_FOLDER_PATH_5 := $(patsubst %/,%,$(dir $(KEYBOARD_FOLDER_PATH_4)))
	KEYBOARD_FOLDER_1 := $(notdir $(KEYBOARD_FOLDER_PATH_1))
	KEYBOARD_FOLDER_2 := $(notdir $(KEYBOARD_FOLDER_PATH_2))
	KEYBOARD_FOLDER_3 := $(notdir $(KEYBOARD_FOLDER_PATH_3))
	KEYBOARD_FOLDER_4 := $(notdir $(KEYBOARD_FOLDER_PATH_4))
	KEYBOARD_FOLDER_5 := $(notdir $(KEYBOARD_FOLDER_PATH_5))
	KEYBOARD_PATHS :=
	KEYBOARD_PATH_1 := keyboards/$(KEYBOARD_FOLDER_PATH_1)
	KEYBOARD_PATH_2 := keyboards/$(KEYBOARD_FOLDER_PATH_2)
	KEYBOARD_PATH_3 := keyboards/$(KEYBOARD_FOLDER_PATH_3)
	KEYBOARD_PATH_4 := keyboards/$(KEYBOARD_FOLDER_PATH_4)
	KEYBOARD_PATH_5 := keyboards/$(KEYBOARD_FOLDER_PATH_5)
	ifneq ("$(wildcard $(KEYBOARD_PATH_5)/)","")
	KEYBOARD_PATHS += $(KEYBOARD_PATH_5)
	endif
	ifneq ("$(wildcard $(KEYBOARD_PATH_4)/)","")
	KEYBOARD_PATHS += $(KEYBOARD_PATH_4)
	endif
	ifneq ("$(wildcard $(KEYBOARD_PATH_3)/)","")
	KEYBOARD_PATHS += $(KEYBOARD_PATH_3)
	endif
	ifneq ("$(wildcard $(KEYBOARD_PATH_2)/)","")
	KEYBOARD_PATHS += $(KEYBOARD_PATH_2)
	endif
	ifneq ("$(wildcard $(KEYBOARD_PATH_1)/)","")
	KEYBOARD_PATHS += $(KEYBOARD_PATH_1)
	endif

Thanks, I did miss this! This doesn't seem to be what was causing the CI issues though, since the bastardkb/charybdis/*/v2/splinky/v3:default firmwares were building fine?

I happy to add to the madness and submit a PR for KEYBOARD_FOLDER_PATH_6 if you want, or try to look into a way to support arbitrary depth.

[fauxpark]

It would be better to rethink your folder structure, keyboard names with lots of nesting can be confusing to users.

[0xcharly]

It would be better to rethink your folder structure, keyboard names with lots of nesting can be confusing to users.

What alternatives are supported by the build system?

The goal of using this structure was to be able to eventually transition to "data-driven" keyboard definitions, where sub-folders could inherit definitions of their parents in a composable way.

The 2 alternatives that I can think of at the moment are:

• Use a flat list of folders, which implies duplicating the shared code, and down the road a lot of the JSON declarations, which risks being brittle and error-prone.
• Use the C pre-processor to support the multiple variations that we have, but that will not be compatible with the "data-driven" approach (as JSON does not have control structures).

As for the potential confusion, AFAICT our experience with users on our Discord server shows that this is not really a problem in practice. I would rather be certain that this structure is a source of confusion before spending time reworking it, and confusing existing users that are already familiar with the current structure.

[fauxpark]

I made a suggestion here: #19088 (comment)

[0xcharly]

I have a follow-up PR to re-work the Splinky versioning (0xcharly#2). Let me know if you prefer to keep it as a follow-up, which I'll send as soon as this PR is merged, or if you'd rather merge them both.

[0xcharly]

0xcharly requested review from drashna and keyboard-magpie and removed request for fauxpark, drashna and keyboard-magpie

That makes no sense to me, I just clicked the request review icon on Drashna…

[drashna]

0xcharly requested review from drashna and keyboard-magpie and removed request for fauxpark, drashna and keyboard-magpie

That makes no sense to me, I just clicked to request review icon on Drashna…

i ... yeah, wow. WTF github. It sometimes does some very weird stuff...

[drashna]

I think this is one of those weird issues.

using bastardkb/charybdis/4x6/v2/splinky/v3 as an example

IIRC, and from what the code appears to read as, it reads for the furthest down (eg v3) and gets each layer higher. However, this means that anything in the 6th from the bottom or further doesn't get processed. So nothing in bastardkb gets processed (not included in vpath, not included rules/confg, etc. So anything in that folter is actively ignored, basically.

Right now, that's not breaking anything, at least from the compiler standpoint. But any deeper, and it would ignore the charybdis config and files (including charybdis.c).

For instance, the KEYBOARD_bastardkb_charybdis define is generated, but KEYBOARD_bastardkb is not.

[0xcharly]

Thanks! Follow-up PR to address splinky versioning and folder hierarchy issues: #19123