Initialize Layer State on startup

[drashna]

Right now, on startup, the default layer state gets called and set, triggering the callback functions for the default layer state. However, the normal layer state never actually gets initialized. It's set to 0 directly, by default, but the callback functions are never actually called. This creates some inconsistency in the behavior for end users. This adds a simple "clear" that triggers the callback on startup. This should produce more consistent behavior between the two functions and layer masks.

Types of Changes

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

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

[drashna]

hmm, only issue is that the mock test now fails.

Probably why this wasn't implemented before.
Unfortunately, I'm not sure how to fix the test. :(

[fauxpark]

Haven't we run into this before?

[drashna]

Yes. Specifically with re-initing on eeprom reset, IIRC. But it was failing at a different point.

Honestly, I suspect that the testing could use some TLC.

[kitlaan]

While I'm not sure what the intent of the gunit tests are... This causes them to go away with your original change:

diff --git a/tests/test_common/test_fixture.cpp b/tests/test_common/test_fixture.cpp
index 8caf1fca4..926b31512 100644
--- a/tests/test_common/test_fixture.cpp
+++ b/tests/test_common/test_fixture.cpp
@@ -22,7 +22,8 @@ using testing::Return;
 
 void TestFixture::SetUpTestCase() {
     TestDriver driver;
-    EXPECT_CALL(driver, send_keyboard_mock(_));
+    // Once for initial initialization, and again for bootmagic init
+    EXPECT_CALL(driver, send_keyboard_mock(_)).Times(2);
     keyboard_init();
 }
 
@@ -32,6 +33,7 @@ TestFixture::TestFixture() {}
 
 TestFixture::~TestFixture() {
     TestDriver driver;
+    EXPECT_CALL(driver, send_keyboard_mock(_)).Times(1);
     layer_clear();
     clear_all_keys();
     // Run for a while to make sure all keys are completely released
~

[spidey3]

Just curious, shouldn't this also be done even if bootmagic or magic are not configured (e.g. if using bootmagic lite)?

[drashna]

Just curious, shouldn't this also be done even if bootmagic or magic are not configured (e.g. if using bootmagic lite)?

Nope.

If bootmagic is set to:

• full/yes: bootmagic.c is included
• lite: magic.c is included
• no (default): magic.c is included

So there is no circumstance at which one or the other isn't included.

The logic for this is here:

qmk_firmware/tmk_core/common.mk

Lines 29 to 49 in dc6fe85

	# Option modules
	BOOTMAGIC_ENABLE ?= no
	VALID_MAGIC_TYPES := yes full lite
	ifneq ($(strip $(BOOTMAGIC_ENABLE)), no)
	ifeq ($(filter $(BOOTMAGIC_ENABLE),$(VALID_MAGIC_TYPES)),)
	$(error BOOTMAGIC_ENABLE="$(BOOTMAGIC_ENABLE)" is not a valid type of magic)
	endif
	ifeq ($(strip $(BOOTMAGIC_ENABLE)), lite)
	TMK_COMMON_DEFS += -DBOOTMAGIC_LITE
	TMK_COMMON_SRC += $(COMMON_DIR)/bootmagic_lite.c
	TMK_COMMON_DEFS += -DMAGIC_ENABLE
	TMK_COMMON_SRC += $(COMMON_DIR)/magic.c
	else
	TMK_COMMON_DEFS += -DBOOTMAGIC_ENABLE
	TMK_COMMON_SRC += $(COMMON_DIR)/bootmagic.c
	endif
	else
	TMK_COMMON_DEFS += -DMAGIC_ENABLE
	TMK_COMMON_SRC += $(COMMON_DIR)/magic.c
	endif

[spidey3]

So there is no circumstance at which one or the other isn't included.

I stand corrected!
Hadn't noticed that magic.c would be included in all circumstances.

[drashna]

No worries! There are a lot of things that are hidden and obfuscated, if you haven't dug deep into the bowels of the code. That's part of why I posted the code reference, since learning more == good. :)

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

[tzarc]

Retargeted develop due to core changes.
Are we waiting on anything else other than another review?

[drashna]

Are we waiting on anything else other than another review?

Nope. Should be ready to merge.