Refactor and updates to TKC1800 code #8472

TerryMathews · 2020-03-18T06:39:17Z

Updates to support QMK-DFU bootloader, and refactor of OLED support to utilize built-in I2C and OLED drivers.

Description

Updated existing keymaps to remove extraneous OLED code.

Types of Changes

Issues Fixed or Closed by This PR

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

Looks good to me.

TerryMathews · 2020-03-18T18:03:49Z

Conflict resolved.

TerryMathews · 2020-03-18T18:20:38Z

Missed that the keymap file in the via folder needed the revisions for OLED as well since I eliminated the non-standardized I2C and OLED includes. Tested and working.

zvecr

https://docs.qmk.fm/#/breaking_changes_instructions

As this modifies user keymaps, we either need to get sign off from @yanfali and @smt.

TerryMathews · 2020-03-19T01:43:40Z

While I did modify the OLED code in their keymaps (as it would no longer compile without it), this is certainly not a breaking change. Their keymaps are completely unaltered. I was even careful to carry over the same layer labels on smt's map.

zvecr · 2020-03-19T01:49:43Z

You might have a different view for what the terminology would mean, however it falls under the following in the linked docs:

Edits to User Keymaps A user may submit their keymap to QMK, then some time later open a pull request with further updates, only to find it can’t be merged because it was edited in the qmk/qmk_firmware repository. As not all users are proficient at using Git or GitHub, the user may find themself unable to fix the issue on their own.

wilba · 2020-03-19T01:55:33Z

@zvecr I know this is not the place to argue this point, but seriously, if you know enough git and github to do a pull request and commit your keymap to a public repo then you ought to know how to resolve a merge conflict, and if you don't, you have no business doing the PR in the first place.

TerryMathews · 2020-03-19T01:57:47Z

@zvecr That's fine, we can certainly agree to disagree... If you're going to require signoff from all user keymap contributors, you failed to include @Wilba6582

skullydazed · 2020-03-19T02:11:48Z

The breaking changes process is not just about the user. People who use this in keymaps that have never been submitted to us may need to make a similar change, and breaking changes is part of how we communicate those changes to users.

skullydazed · 2020-03-19T02:23:03Z

We are human and don't always catch these things.

zvecr · 2020-03-19T02:23:24Z

For context, on that M0lly one i went by the "not shipped" status on https://thekey.company/products/m0lly (and the lack of user keymaps which might have been wrong)

TerryMathews · 2020-03-19T02:34:16Z

I'm sorry for getting short. It's just aggravating; I put in a not inconsequential amount of time throwing away perfectly good code at your (regal form) request to move both codebases to using the in-built OLED and I2C drivers.

skullydazed · 2020-03-19T19:28:12Z

I definitely understand why it's frustrating. We're trying to balance the needs of two different sets of users- keyboard maintainers and end users. Sometimes we have to choose which group will experience more pain and we will usually choose to avoid end user pain wherever possible.

Thanks for your work here. In the end having a uniform interface for oleds will make things like configurator support possible, and that benefits both of us.

TerryMathews · 2020-03-19T19:35:05Z

No I understand the need for it to be done, which is why it was on our punch list of code updates to be done.

yanfali · 2020-03-28T02:38:44Z

keyboards/tkc1800/rules.mk

@@ -9,12 +9,13 @@ MCU = at90usb1286
 #   QMK DFU      qmk-dfu
 #   ATmega32A    bootloadHID
 #   ATmega328P   USBasp
-BOOTLOADER = atmel-dfu
+BOOTLOADER = qmk-dfu


I'm fine with the keymap changes. This looks like a much cleaner way to disable i2c, which is incompatible with console logging if you didn't have the OLED.

My only concern here is that all the older PCBs in existence, are still be running atmel-dfu, which has a slightly different behavior around erasure. Usually how this handled is PCB rev folders. I haven't tested what happens if I try to RESET with the bootloader set to qmk-dfu instead of atmel-dfu.

I can certainly add a rules.mk override so this isn't a blocker, more of a technical question. I can also ISP flash my pcb and add qmk-dfu.

@yanfali I've tested it, works without issue. Having the QMK code setup for qmk-dfu works correctly whether the bootloader is qmk-dfu or atmel-dfu.

yanfali

commented, but otherwise fine with keymap changes.

TerryMathews · 2020-04-29T18:37:35Z

I don't see #1 as a valid option. Reverting it will cause his/her keymap to no longer compile, and the build test will fail. As presented in the PR, his/her keymap compiles and works as (s)he would expect it to.

noroadsleft · 2020-04-30T06:39:30Z

I don't see #1 as a valid option. Reverting it will cause his/her keymap to no longer compile, and the build test will fail. As presented in the PR, his/her keymap compiles and works as (s)he would expect it to.

Very well.

If you can write a summary of how a TKC1800 user who doesn't have their keymap merged to qmk:master can update their keymap to be compatible with this code change, and stick it in qmk_firmware/docs/ChangeLog/20200530/PR8472.md, I'll merge this to the future branch and it'll hit QMK mainline in about a month's time.

smt · 2020-05-03T22:50:46Z

I will accept the conflict, please don’t let my keymap conflict stop you from putting in this improvement.

…

On Thu, Apr 30, 2020 at 2:39 AM, James Young ***@***.***> wrote: > I don't see [#1](#1) as a valid option. Reverting it will cause his/her keymap to no longer compile, and the build test will fail. As presented in the PR, his/her keymap compiles and works as (s)he would expect it to. Very well. If you can write a summary of how a TKC1800 user who doesn't have their keymap merged to qmk:master can update their keymap to be compatible with this code change, and stick it in qmk_firmware/docs/ChangeLog/20200530/PR8472.md, I'll merge this to the future branch and it'll hit QMK mainline in about a month's time. — You are receiving this because you were mentioned. Reply to this email directly, [view it on GitHub](#8472 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAABJAPA5YSEANPLMSY62DLRPEMLBANCNFSM4LOIETUQ).

TerryMathews · 2020-05-04T01:16:02Z

@noroadsleft Now that I have approval from @smt as well, do I still need the ChangeLog entry? @zvecr said in #8472 (review) that I needed sign-off from those two and I have it.

noroadsleft · 2020-05-04T06:26:33Z

@TerryMathews The reason for the changelog is so that we can document instructions for users who don't have their keymaps hosted in the repository, and thus may need instructions on how to fix their keymaps so they compile once again.

Just that one change and I'll merge this.

TerryMathews · 2020-05-04T22:47:04Z

@noroadsleft Done.

noroadsleft · 2020-05-05T16:09:18Z

@TerryMathews Wonderful work on the ChangeLog. Thanks!

* TKC1800: updated to support QMK-DFU bootloader * TKC1800: updated to support QMK-DFU bootloader * TKC1800: Updated to utilize common I2C and OLED code * Refactor OLED code in VIA keymap * Fix for screen disconnected error. * Added changelog for breaking change process

* Branch point for 2020 May 30 Breaking Change * Migrate `ACTION_LAYER_TOGGLE` to `TG()` (#8954) * Migrate `ACTION_MODS_ONESHOT` to `OSM()` (#8957) * Migrate `ACTION_DEFAULT_LAYER_SET` to `DF()` (#8958) * Migrate `ACTION_LAYER_MODS` to `LM()` (#8959) * Migrate `ACTION_MODS_TAP_KEY` to `MT()` (#8968) * Convert V-USB usbdrv to a submodule (#8321) * Unify Tap Hold functions and documentation (#8348) * Changing board names to prevent confusion (#8412) * Move the Keyboardio Model01 to a keyboardio/ subdir (#8499) * Move spaceman keyboards (#8830) * Migrate miscellaneous `fn_actions` entries (#8977) * Migrate `ACTION_MODS_KEY` to chained mod keycodes (#8979) * Organizing my keyboards (plaid, tartan, ergoinu) (#8537) * Refactor Lily58 to use split_common (#6260) * Refactor zinc to use split_common (#7114) * Add a message if bin/qmk doesn't work (#9000) * Fix conflicting types for 'tfp_printf' (#8269) * Fixed RGB_DISABLE_AFTER_TIMEOUT to be seconds based & small internals cleanup (#6480) * Refactor and updates to TKC1800 code (#8472) * Switch to qmk forks for everything (#9019) * audio refactor: replace deprecated PLAY_NOTE_ARRAY (#8484) * Audio enable corrections (2/3) (#8903) * Split HHKB to ANSI and JP layouts and Add VIA support for each (#8582) * Audio enable corrections (Part 4) (#8974) * Fix typo from PR7114 (#9171) * Augment future branch Changelogs (#8978) * Revert "Branch point for 2020 May 30 Breaking Change"

* Branch point for 2020 May 30 Breaking Change * Migrate `ACTION_LAYER_TOGGLE` to `TG()` (qmk#8954) * Migrate `ACTION_MODS_ONESHOT` to `OSM()` (qmk#8957) * Migrate `ACTION_DEFAULT_LAYER_SET` to `DF()` (qmk#8958) * Migrate `ACTION_LAYER_MODS` to `LM()` (qmk#8959) * Migrate `ACTION_MODS_TAP_KEY` to `MT()` (qmk#8968) * Convert V-USB usbdrv to a submodule (qmk#8321) * Unify Tap Hold functions and documentation (qmk#8348) * Changing board names to prevent confusion (qmk#8412) * Move the Keyboardio Model01 to a keyboardio/ subdir (qmk#8499) * Move spaceman keyboards (qmk#8830) * Migrate miscellaneous `fn_actions` entries (qmk#8977) * Migrate `ACTION_MODS_KEY` to chained mod keycodes (qmk#8979) * Organizing my keyboards (plaid, tartan, ergoinu) (qmk#8537) * Refactor Lily58 to use split_common (qmk#6260) * Refactor zinc to use split_common (qmk#7114) * Add a message if bin/qmk doesn't work (qmk#9000) * Fix conflicting types for 'tfp_printf' (qmk#8269) * Fixed RGB_DISABLE_AFTER_TIMEOUT to be seconds based & small internals cleanup (qmk#6480) * Refactor and updates to TKC1800 code (qmk#8472) * Switch to qmk forks for everything (qmk#9019) * audio refactor: replace deprecated PLAY_NOTE_ARRAY (qmk#8484) * Audio enable corrections (2/3) (qmk#8903) * Split HHKB to ANSI and JP layouts and Add VIA support for each (qmk#8582) * Audio enable corrections (Part 4) (qmk#8974) * Fix typo from PR7114 (qmk#9171) * Augment future branch Changelogs (qmk#8978) * Revert "Branch point for 2020 May 30 Breaking Change"

TerryMathews added 3 commits March 17, 2020 18:48

TKC1800: updated to support QMK-DFU bootloader

4525048

TKC1800: updated to support QMK-DFU bootloader

716d677

TKC1800: Updated to utilize common I2C and OLED code

8c048a2

wilba mentioned this pull request Mar 18, 2020

VIA support for TKC1800 #8178

Merged

13 tasks

Erovia added keyboard keymap labels Mar 18, 2020

Erovia requested a review from a team March 18, 2020 09:25

drashna approved these changes Mar 18, 2020

View reviewed changes

drashna requested a review from a team March 18, 2020 16:51

fauxpark approved these changes Mar 18, 2020

View reviewed changes

Merge branch 'master' into tkc1800

e5f8be0

Refactor OLED code in VIA keymap

264231e

zvecr added the breaking_change Changes that need to wait for a version increment label Mar 19, 2020

zvecr requested changes Mar 19, 2020

View reviewed changes

drashna self-requested a review March 19, 2020 04:08

drashna requested a review from a team March 21, 2020 06:30

yanfali reviewed Mar 28, 2020

View reviewed changes

yanfali approved these changes Mar 28, 2020

View reviewed changes

TerryMathews added 2 commits May 4, 2020 18:37

Fix for screen disconnected error.

915f9d4

Added changelog for breaking change process

0ef1d42

noroadsleft changed the base branch from master to future May 5, 2020 16:09

noroadsleft added the future_2020q2 label May 5, 2020

noroadsleft merged commit bba89c6 into qmk:future May 5, 2020

TerryMathews deleted the tkc1800 branch May 7, 2020 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and updates to TKC1800 code #8472

Refactor and updates to TKC1800 code #8472

TerryMathews commented Mar 18, 2020

drashna left a comment

TerryMathews commented Mar 18, 2020

TerryMathews commented Mar 18, 2020

zvecr left a comment

TerryMathews commented Mar 19, 2020

zvecr commented Mar 19, 2020

wilba commented Mar 19, 2020

TerryMathews commented Mar 19, 2020

skullydazed commented Mar 19, 2020

skullydazed commented Mar 19, 2020

zvecr commented Mar 19, 2020

TerryMathews commented Mar 19, 2020

skullydazed commented Mar 19, 2020

TerryMathews commented Mar 19, 2020

yanfali Mar 28, 2020 •

edited

Loading

TerryMathews Mar 28, 2020

yanfali left a comment

TerryMathews commented Apr 29, 2020

noroadsleft commented Apr 30, 2020

smt commented May 3, 2020 via email

TerryMathews commented May 4, 2020

noroadsleft commented May 4, 2020

TerryMathews commented May 4, 2020

noroadsleft commented May 5, 2020

Refactor and updates to TKC1800 code #8472

Refactor and updates to TKC1800 code #8472

Conversation

TerryMathews commented Mar 18, 2020

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

drashna left a comment

Choose a reason for hiding this comment

TerryMathews commented Mar 18, 2020

TerryMathews commented Mar 18, 2020

zvecr left a comment

Choose a reason for hiding this comment

TerryMathews commented Mar 19, 2020

zvecr commented Mar 19, 2020

wilba commented Mar 19, 2020

TerryMathews commented Mar 19, 2020

skullydazed commented Mar 19, 2020

skullydazed commented Mar 19, 2020

zvecr commented Mar 19, 2020

TerryMathews commented Mar 19, 2020

skullydazed commented Mar 19, 2020

TerryMathews commented Mar 19, 2020

yanfali Mar 28, 2020 • edited Loading

Choose a reason for hiding this comment

TerryMathews Mar 28, 2020

Choose a reason for hiding this comment

yanfali left a comment

Choose a reason for hiding this comment

TerryMathews commented Apr 29, 2020

noroadsleft commented Apr 30, 2020

smt commented May 3, 2020 via email

TerryMathews commented May 4, 2020

noroadsleft commented May 4, 2020

TerryMathews commented May 4, 2020

noroadsleft commented May 5, 2020

yanfali Mar 28, 2020 •

edited

Loading