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

Rebound: add rev2 and thus rev1 as well #8630

Merged
merged 12 commits into from
Apr 21, 2020
Merged

Rebound: add rev2 and thus rev1 as well #8630

merged 12 commits into from
Apr 21, 2020

Conversation

Rossman360
Copy link
Contributor

@Rossman360 Rossman360 commented Mar 31, 2020 •
edited by drashna
Loading

I made a rev2 for Rebound, a 49-key version. Then, I made rev1 files for the 48-key version and created default keymaps for each

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.
  • 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
drashna reviewed Mar 31, 2020
View reviewed changes
Copy link
Member

@drashna drashna left a comment

Choose a reason for hiding this comment

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

Just a couple of nit picks. but otherwise, looks good.

@drashna
Copy link
Member

drashna commented Apr 2, 2020

Okay, taking a closer look at this, both revisions lack a "default" keymap.

Additionally, the revision 2 errors out with the default community layout (too many keys). And it looks like it doesn't actually match the 5x12 or 4x12 ortho layouts. And if that is the case, support (and naming) for it should be removed.

@drashna drashna requested a review from a team April 2, 2020 06:51
@Rossman360
Copy link
Contributor Author

Rossman360 commented Apr 2, 2020 •
edited
Loading

"buncha stuff" PR

-renamed ortho_5x12 to "49" and ortho_4x12 to "48" to reflect the number of keys rather than how the matrix is set up.
-made rev2 default
-added rotary pins for rev2
-renamed default49 keymap to default
-set up rev1 so it doesn't fail with 49-key keymap (still unsure how to set a default keymap for a specific rev)

also added to rebound.h:

#ifdef KEYBOARD_montsinger_rebound_rev1
#include "rev1.h"
#elif KEYBOARD_montsinger_rebound_rev2
#include "rev2.h"
#endif

not sure what it does, but it looked important in the Iris keymap

@Rossman360
Copy link
Contributor Author

hang on, I'm changing everything

zvecr
zvecr requested changes Apr 2, 2020
View reviewed changes
Copy link
Member

@zvecr zvecr left a comment

Choose a reason for hiding this comment

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

Theres a fair amount here to unwind, but has rev1 physically gained an extra row? if not then may mean you need a keymap folder under each revision

@Rossman360
Copy link
Contributor Author

Rossman360 commented Apr 2, 2020 •
edited
Loading

I redid everything so that there is only one rev, added an info.json with both available layouts, renamed the layouts to LAYOUT (49 key) and LAYOUT_48. The default48 keymap disables the rotary encoder and utilizes the appropriate layout for the four people who own that version of the board.

zvecr
zvecr requested changes Apr 2, 2020
View reviewed changes
@Rossman360
Copy link
Contributor Author

I think I've made the suggested changes, but I get errors compiling the default48 keymap because of the implicit declaration of the community layout, when it wasn't doing that when I had named given it a custom name

zvecr
zvecr requested changes Apr 2, 2020
View reviewed changes
noroadsleft
noroadsleft requested changes Apr 4, 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.

From the looks of things, this really should be done as two revisions.

noroadsleft@3c03938

This should work for both revisions. The codebases for each are split up, as are the keymaps.

noroadsleft
noroadsleft approved these changes Apr 5, 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.

Looks good to me.

zvecr
zvecr requested changes Apr 5, 2020
View reviewed changes
Copy link
Member

@zvecr zvecr left a comment

Choose a reason for hiding this comment

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

Can we get sign off from @Curry due to the keymap move

Curry
Curry approved these changes Apr 5, 2020
View reviewed changes
Copy link
Contributor

@Curry Curry 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.

Keymap move ok 😄

@Rossman360
Copy link
Contributor Author

anything needed from me at this time?

@Erovia
Copy link
Member

Erovia commented Apr 8, 2020

You could also create a keyboard level rules.mk and use the DEFAULT_FOLDER directive. With this the rebound build target will not brake for users.
You can check the lets_split files for more detail.

@fauxpark fauxpark changed the title add rev2 and thus rev1 as well Rebound: add rev2 and thus rev1 as well Apr 8, 2020
noroadsleft
noroadsleft requested changes Apr 9, 2020
View reviewed changes
@Rossman360 Rossman360 requested a review from zvecr April 14, 2020 12:32
@zvecr zvecr merged commit 738c661 into qmk:master Apr 21, 2020
@zvecr
Copy link
Member

zvecr commented Apr 21, 2020

Thanks!

rockwarrior added a commit to rockwarrior/qmk_firmware that referenced this pull request Apr 23, 2020
@rockwarrior
Merge branch 'upstream'
dff8c1b 
* upstream: (1037 commits)
  [Keyboard] Add wasdat code controller (qmk#8858)
  fix sample code indent in feature_encoders.md (qmk#8883)
  [Keymap] Clean up my ergo keymaps and userspace (qmk#8857)
  idb 60 Bugfixes / Preparations for Open Source Hardware (qmk#8866)
  Rebound: add rev2 and thus rev1 as well (qmk#8630)
  Update vitamins included default keymap, enable NKRO, rev2 rgbsplit (qmk#8871)
  Update to xealousbrown. (qmk#8215)
  [Keyboard] DMQ Design SPIN (qmk#8820)
  Wheatfield Blocked65: Update RGBLED num (qmk#8725)
  Add VIA support to ID80 (qmk#8791)
  CFTKB Mysterium & Discipad VIA support (qmk#8794)
  Clean up ATSAM ifdefs (qmk#8808)
  [Docs] Japanese translation of docs/feature_encoders.md (qmk#8843)
  Add naked60 layout, clean up my userspace files and rules.mk. (qmk#8848)
  Fixing DecadePad Numlock LED Bug (qmk#8831)
  [Docs] Japanese translation of docs/feature_command.md (qmk#8672)
  Add support for YMD75 rev 2 (qmk#8853)
  Remove no-longer-necessary LTO checks from keyboards' config.h files (qmk#8773)
  Fix ta-65 tsangan layouts (qmk#8855)
  Fix Plain60 layout (qmk#8854)
  ...
violet-fish pushed a commit to violet-fish/qmk_firmware that referenced this pull request May 3, 2020
@Rossman360 @violet-fish
Rebound: add rev2 and thus rev1 as well (qmk#8630)
db21aa1 
* add rev2 and thus rev1 as well

* nitpicks :)

* buncha stuff

* back to one rev

* back to community layout with errors

* I see you've met my typo

* remove default48 kemap rules

* re-rework into 2 revs

* readme changes

* whitespace cleanup

* default folder

* rev1 be default
bitherder pushed a commit to bitherder/qmk_firmware that referenced this pull request May 15, 2020
@Rossman360 @lorenzoernesto
Rebound: add rev2 and thus rev1 as well (qmk#8630)
f412bb7 
* add rev2 and thus rev1 as well

* nitpicks :)

* buncha stuff

* back to one rev

* back to community layout with errors

* I see you've met my typo

* remove default48 kemap rules

* re-rework into 2 revs

* readme changes

* whitespace cleanup

* default folder

* rev1 be default
sowbug pushed a commit to sowbug/qmk_firmware that referenced this pull request May 24, 2020
@Rossman360 @sowbug
Rebound: add rev2 and thus rev1 as well (qmk#8630)
d097c85 
* add rev2 and thus rev1 as well

* nitpicks :)

* buncha stuff

* back to one rev

* back to community layout with errors

* I see you've met my typo

* remove default48 kemap rules

* re-rework into 2 revs

* readme changes

* whitespace cleanup

* default folder

* rev1 be default
turky pushed a commit to turky/qmk_firmware that referenced this pull request Jun 13, 2020
@Rossman360 @turky
Rebound: add rev2 and thus rev1 as well (qmk#8630)
b9714ce 
* add rev2 and thus rev1 as well

* nitpicks :)

* buncha stuff

* back to one rev

* back to community layout with errors

* I see you've met my typo

* remove default48 kemap rules

* re-rework into 2 revs

* readme changes

* whitespace cleanup

* default folder

* rev1 be default
jakobaa pushed a commit to jakobaa/qmk_firmware that referenced this pull request Jul 7, 2020
@Rossman360 @jakobaa
Rebound: add rev2 and thus rev1 as well (qmk#8630)
71e722c 
* add rev2 and thus rev1 as well

* nitpicks :)

* buncha stuff

* back to one rev

* back to community layout with errors

* I see you've met my typo

* remove default48 kemap rules

* re-rework into 2 revs

* readme changes

* whitespace cleanup

* default folder

* rev1 be default
sjmacneil pushed a commit to sjmacneil/qmk_firmware that referenced this pull request Feb 19, 2021
@Rossman360 @sjmacneil
Rebound: add rev2 and thus rev1 as well (qmk#8630)
97464a8 
* add rev2 and thus rev1 as well

* nitpicks :)

* buncha stuff

* back to one rev

* back to community layout with errors

* I see you've met my typo

* remove default48 kemap rules

* re-rework into 2 revs

* readme changes

* whitespace cleanup

* default folder

* rev1 be default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drashna drashna drashna left review comments

@Curry Curry Curry approved these changes

@zvecr zvecr zvecr approved these changes

@noroadsleft noroadsleft noroadsleft approved these changes

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

Successfully merging this pull request may close these issues.
6 participants
@Rossman360 @drashna @Erovia @zvecr @Curry @noroadsleft