Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

_33 v2 Keyboard #9899

Merged
merged 13 commits into from
Aug 25, 2020
Merged

_33 v2 Keyboard #9899

merged 13 commits into from
Aug 25, 2020

Conversation

tominabox1
Copy link
Contributor

Description

Adding Rev2 version of this keyboard and VIA keymap.

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

@tominabox1
Copy link
Contributor Author

tominabox1 commented Aug 1, 2020 •
edited
Loading

Looks like the Lint check is failing the qmk info check because I have DEFAULT_FOLDER = underscore33/rev2 in my top-level rules.mk. I followed this based on other boards but I may be the first to have different matrices in the revs that would cause this. Is there some advise to take care of this or does it matter? It looks ok in the configurator test and builds fine.

drashna
drashna approved these changes Aug 5, 2020
View reviewed changes
Copy link
Member

@drashna drashna left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I lied. Travis CI has some issues.

@tominabox1
Changed default keymap for r1 to match new split bottom row macro nam…
4ca18a9 
…e, updated via keymap readme, updated r1 json to match layout macro name, updated split space macro for r1
fauxpark
fauxpark requested changes Aug 8, 2020
View reviewed changes
keyboards/underscore33/rev1/rules.mk Outdated Show resolved Hide resolved
keyboards/underscore33/rev2/rules.mk Outdated Show resolved Hide resolved
keyboards/underscore33/rev2/rules.mk Outdated Show resolved Hide resolved
@tominabox1 tominabox1 mentioned this pull request Aug 13, 2020
Closed
6 tasks
@tominabox1
Copy link
Contributor Author

is there something I still need to do to move this forward? I think I've dealt with all the outstanding issues.

fauxpark
fauxpark requested changes Aug 21, 2020
View reviewed changes
Copy link
Member

@fauxpark fauxpark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just these two things, I think.

keyboards/underscore33/rev2/rules.mk Outdated Show resolved Hide resolved
keyboards/underscore33/rev1/rules.mk Outdated Show resolved Hide resolved
@fauxpark fauxpark added breaking_change Changes that need to wait for a version increment keyboard keymap labels Aug 21, 2020
@fauxpark fauxpark requested a review from a team August 21, 2020 01:01
@fauxpark fauxpark removed the breaking_change Changes that need to wait for a version increment label Aug 21, 2020
noroadsleft
noroadsleft requested changes Aug 24, 2020
View reviewed changes
Copy link
Member

@noroadsleft noroadsleft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the change in split space layout from rev1 to rev2, I'd much prefer making the keymaps revision specific (underscore33/rev1/keymaps/ and underscore33/rev2/keymaps/, instead of just underscore33/keymaps/). This has the advantage that you could add VIA support for both revisions, and also maintain compatibility for anyone with a rev1 PCB.

As currently implemented, this breaks QMK Configurator exports for anyone with a rev1 using the Split Space layout.

noroadsleft
noroadsleft requested changes Aug 25, 2020
View reviewed changes
Copy link
Member

@noroadsleft noroadsleft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub tip: You can apply multiple suggestions to a single commit by using the Files Changed tab.

Sorry for the multiple review cycles.

keyboards/underscore33/rev1/rev1.h Outdated Show resolved Hide resolved
keyboards/underscore33/rev1/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/underscore33/rev1/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/underscore33/rev1/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/underscore33/rev1/keymaps/via/keymap.c Outdated Show resolved Hide resolved
keyboards/underscore33/rev2/keymaps/default/readme.md Outdated Show resolved Hide resolved
keyboards/underscore33/rev2/keymaps/via/readme.md Outdated Show resolved Hide resolved
keyboards/underscore33/rules.mk Outdated Show resolved Hide resolved
keyboards/underscore33/rev1/config.h Outdated Show resolved Hide resolved
keyboards/underscore33/rev2/config.h Outdated Show resolved Hide resolved
@tominabox1
Copy link
Contributor Author

GitHub tip: You can apply multiple suggestions to a single commit by using the Files Changed tab.

Sorry for the multiple review cycles.

No problem, not done a revision before so glad to get it right.

@noroadsleft noroadsleft merged commit eb84f13 into qmk:master Aug 25, 2020
@noroadsleft
Copy link
Member

Thanks!

nicocesar pushed a commit to nicocesar/qmk_firmware that referenced this pull request Sep 6, 2020
@tominabox1 @nicocesar
[Keyboard] _33 v2 (qmk#9899)
3172121 
* Initial prep for PR

* Fixing jsons for revs

* Remove old keymap ref in readme

* Add Rev1 default layout

* Fix extra comma in default r1 keymap

* Changed default keymap for r1 to match new split bottom row macro name, updated via keymap readme, updated r1 json to match layout macro name, updated split space macro for r1

* Moved combo configs to default keymaps, removed unused bootloader selections

* Update keyboards/underscore33/rev1/rules.mk

* Update keyboards/underscore33/rev2/rules.mk

* Refactor _33 folder structure

* Add VIA keymap to rev1

* Rename macros and product_id as suggested
@tominabox1 tominabox1 mentioned this pull request Sep 16, 2020
Merged
6 tasks
kjganz pushed a commit to kjganz/qmk_firmware that referenced this pull request Oct 28, 2020
@tominabox1
[Keyboard] _33 v2 (qmk#9899)
850e039 
* Initial prep for PR

* Fixing jsons for revs

* Remove old keymap ref in readme

* Add Rev1 default layout

* Fix extra comma in default r1 keymap

* Changed default keymap for r1 to match new split bottom row macro name, updated via keymap readme, updated r1 json to match layout macro name, updated split space macro for r1

* Moved combo configs to default keymaps, removed unused bootloader selections

* Update keyboards/underscore33/rev1/rules.mk

* Update keyboards/underscore33/rev2/rules.mk

* Refactor _33 folder structure

* Add VIA keymap to rev1

* Rename macros and product_id as suggested
@tominabox1 tominabox1 deleted the underscore33_v2 branch December 9, 2020 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drashna drashna drashna approved these changes

@fauxpark fauxpark fauxpark requested changes

@noroadsleft noroadsleft noroadsleft approved these changes

Assignees
No one assigned
Labels
keyboard keymap
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tominabox1 @noroadsleft @drashna @fauxpark