Fix KBDPad MKI backlight, Num Lock LEDs; clean up firmware a bit #6883

bcat · 2019-10-03T05:26:42Z

Description

Made Num Lock LED work. It's wired to its own I/O pin and not controlled by the regular backlight pin. (Tested on physical PCB ;Num Lock LED works independent of backlight.)
Use standard backlight implementation, including hardware PWM for breathing. (Tested on physical PCB; brightness and breathing work.)
Replace RGB underglow with standard ps2avrgb driver and add appropriate config so it just works if someone enables it. Leaving it off by default since the stock case makes the underglow almost invisible (but the LEDs are there on the PCB).
Turned off command by default since the QMK docs caution against having it default on, and there wasn't a working IS_COMMAND defined anyway. (No shift keys, so the default doesn't work.)
Remove some unnecessary/default config options.
Add store URL to QMK Configurator and readme.

Types of Changes

Issues Fixed or Closed by This PR

KBDPad MKI doesn't support numlock LED.
KBDPad MKI doesn't support backlight brightness control or breathing.

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

bcat · 2019-10-03T05:27:43Z

If someone can tell me how to test the info.json change locally, that would be much appreciated. :) I copied the layout JSON from another keeb with identical layout support, so it should be fine, but I'd feel better having tested it.

keyboards/kbdfans/kbdpad/mk1/mk1.c

fauxpark · 2019-10-03T05:43:53Z

You can test the info.json by hitting Control+Shift+I in the Configurator :)

keyboards/kbdfans/kbdpad/mk1/mk1.h

keyboards/kbdfans/kbdpad/mk1/rules.mk

yanfali

Don't remove LAYOUTs, that breaks configurator default keymaps in configurator. You can use the #define LAYOUT trick in the layout macro list.

bcat · 2019-10-03T21:51:27Z

@fauxpark Confirmed info.json loads in configurator, thanks!

@yanfali I am not sure I understand, sorry. Does the configurator requires a layout called LAYOUT? Or is it that a layout can't ever be renamed? (I assume as long as the merged keymaps in the QMK repo still built, we were good, but I guess probably lots of folks save JSON from the configurator and never check it in.)

I'm happy to re-add the old layout macro in addition to the ortho_6x4 (the "all" layout for this PCB) and numpad_6x4 layouts, but does this mean I'm stuck calling it LAYOUT? I guess if I rename it this will also break the configurator?

Thinking a little bit more about this, maybe I should split this PR into two: one for the backlight fixes and mk1.c cleanups, and the other for the layout and info.json changes. But I don't have a good sense of desirable PR size for QMK, so I'm good either way.

yanfali · 2019-10-03T23:02:03Z

@bcat it's because configurator can't get keymaps reliably from the API at this time, so there's a separate repo in configurator which contains default keymaps for every keyboard. If you remove a layout that keymap will not load and break configurator. For cases like that we recommed you add a #define alias in the keyboard.h file that maps the old name to the new one to satisfy the API.

e.g.

#define LAYOUT LAYOUT_newly_defined_layout_name_that matches_old_one

we do not recommend changing the default at all, as this just confuses users, it's ok to add new ones, but we are trying to be more backward compatible with old names because we've had a history of arbitrarily breaking them without updating configurator.

Alternately. You can also update the configurator repo, if you are insistent on removing a LAYOUT. But this needs to be done around the same time to avoid breakage in the UI.

fauxpark · 2019-10-03T23:06:44Z

It seems like there is no default keymap submitted for the mkI though, so I don't think there is any issue with that... but you also have the option to import keymap.json files which will break if the layout they specify is missing. There may also be C keymaps in the wild using LAYOUT.

If you want to get rid of it in favour of the more descriptive ones, it should be done in a separate PR that will go through the breaking changes process.

bcat · 2019-10-03T23:58:07Z

@yanfali @fauxpark Thanks, this explanation is super helpful. I didn't actually know there was a separate repo with the configurator defaults, and I also wasn't thinking about JSON keymaps, since those don't get checked into GitHub but it would suck to just break them. So I will go ahead and revert the layout changes for now. (I might send a separate PR later to add the community layouts, but I'll make sure I don't break the existing one.)

Should I put this info somewhere? Maybe the breaking changes docs? As a QMK n00b it wasn't super obvious to me what backwards compatibility obligations existed beyond "make sure the keymaps in the QMK repo still compile". (I see the docs do mention not breaking keymaps, but I wrongly assumed it meant submitted keymaps only.)

bcat · 2019-10-04T04:56:55Z

Reverted layout changes for this PR, sorry for the hassle!

yanfali

Thanks

drashna · 2019-10-08T18:33:17Z

@mechmerlin had some objections here. I'd like to see him weigh in again.

bcat · 2019-10-15T19:12:13Z

Friendly ping for @mechmerlin. I think the only open question was around the RGB changes. I would slightly prefer to have the cleanups in because the PCB does have RGB LEDs wired stock and there's already some RGB code checked in.

However, if you object to this I can revert the RGB changes and just leave the backlight fixes. Up to you, just let me know.

(There's no working IS_COMMAND set for this board anyway, so it's already a nop.)

bcat · 2019-11-02T19:17:38Z

Reworked this PR to use the new ps2avrGB RGB driver. Seems much cleaner now. I can still remove the RGB bits entirely if you folks want, but since all it takes is two config.h settings and one rules.mk setting to make the PCB's built-in RGB ready to use if someone does want it, I'm slightly inclined to leave it in if that's cool with y'all.

I gave my KBDPad away as a gift to a family member and hence while I've built the reworked version of this PR (and there are no changes from the last known working version except the new RGB driver), I have not tested the latest hex yet. Gonna ask said family member if they mind reflashing, but it may be a couple days. (Alternatively, if one of the reviewers has a KBDPad and doesn't mind sanity checking this, that'd be much appreciated. I'm always scared of submitting untested code....)

zvecr

LGTM

fauxpark · 2019-11-03T22:30:10Z

Thanks!

…#6883) * Update keyboard kit URL * Replace custom RGB driver with new one from qmk#7183 * Replace backlight with standard impl * Remove some unnecessary default settings * Disable COMMAND since docs want it off by default (There's no working IS_COMMAND set for this board anyway, so it's already a nop.)

fauxpark reviewed Oct 3, 2019

View reviewed changes

keyboards/kbdfans/kbdpad/mk1/mk1.c Show resolved Hide resolved

fauxpark added keyboard enhancement keymap labels Oct 3, 2019

fauxpark requested a review from a team October 3, 2019 05:44

bcat changed the title ~~Clean up KBDPad MKI firmware and default keymap~~ Fix KBDPad MKI backlight, clean up firmware and default keymap Oct 3, 2019

mechmerlin requested changes Oct 3, 2019

View reviewed changes

keyboards/kbdfans/kbdpad/mk1/mk1.h Outdated Show resolved Hide resolved

keyboards/kbdfans/kbdpad/mk1/rules.mk Outdated Show resolved Hide resolved

yanfali added the configurator affects configurator label Oct 3, 2019

yanfali requested changes Oct 3, 2019

View reviewed changes

bcat changed the title ~~Fix KBDPad MKI backlight, clean up firmware and default keymap~~ Fix KBDPad MKI backlight, Num Lock LEDs; clean up firmware a bit Oct 4, 2019

yanfali approved these changes Oct 4, 2019

View reviewed changes

yanfali removed the configurator affects configurator label Oct 4, 2019

bcat requested a review from mechmerlin October 7, 2019 23:14

fauxpark approved these changes Oct 7, 2019

View reviewed changes

drashna approved these changes Oct 8, 2019

View reviewed changes

bcat added 5 commits November 2, 2019 13:55

Update keyboard kit URL

c79e8ba

Replace custom RGB driver with new one from #7183

35741b2

Replace backlight with standard impl

c9ac7d5

Remove some unnecessary default settings

ef9f37b

Disable COMMAND since docs want it off by default

381f62d

(There's no working IS_COMMAND set for this board anyway, so it's already a nop.)

zvecr approved these changes Nov 2, 2019

View reviewed changes

drashna approved these changes Nov 3, 2019

View reviewed changes

fauxpark merged commit 732d1dd into qmk:master Nov 3, 2019

bcat deleted the bcat_kbdpad branch November 3, 2019 22:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix KBDPad MKI backlight, Num Lock LEDs; clean up firmware a bit #6883

Fix KBDPad MKI backlight, Num Lock LEDs; clean up firmware a bit #6883

bcat commented Oct 3, 2019 •

edited

Loading

bcat commented Oct 3, 2019

fauxpark commented Oct 3, 2019

yanfali left a comment •

edited

Loading

bcat commented Oct 3, 2019 •

edited

Loading

yanfali commented Oct 3, 2019 •

edited

Loading

fauxpark commented Oct 3, 2019 •

edited

Loading

bcat commented Oct 3, 2019

bcat commented Oct 4, 2019

yanfali left a comment

drashna commented Oct 8, 2019

bcat commented Oct 15, 2019

bcat commented Nov 2, 2019 •

edited

Loading

zvecr left a comment

fauxpark commented Nov 3, 2019

Fix KBDPad MKI backlight, Num Lock LEDs; clean up firmware a bit #6883

Fix KBDPad MKI backlight, Num Lock LEDs; clean up firmware a bit #6883

Conversation

bcat commented Oct 3, 2019 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

bcat commented Oct 3, 2019

fauxpark commented Oct 3, 2019

yanfali left a comment • edited Loading

Choose a reason for hiding this comment

bcat commented Oct 3, 2019 • edited Loading

yanfali commented Oct 3, 2019 • edited Loading

fauxpark commented Oct 3, 2019 • edited Loading

bcat commented Oct 3, 2019

bcat commented Oct 4, 2019

yanfali left a comment

Choose a reason for hiding this comment

drashna commented Oct 8, 2019

bcat commented Oct 15, 2019

bcat commented Nov 2, 2019 • edited Loading

zvecr left a comment

Choose a reason for hiding this comment

fauxpark commented Nov 3, 2019

bcat commented Oct 3, 2019 •

edited

Loading

yanfali left a comment •

edited

Loading

bcat commented Oct 3, 2019 •

edited

Loading

yanfali commented Oct 3, 2019 •

edited

Loading

fauxpark commented Oct 3, 2019 •

edited

Loading

bcat commented Nov 2, 2019 •

edited

Loading