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.

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

Add Comet46 keyboard #3342

Merged
merged 8 commits into from
Jul 9, 2018
Merged

Add Comet46 keyboard #3342

merged 8 commits into from
Jul 9, 2018

Conversation

satt99
Copy link
Contributor

@satt99 satt99 commented Jul 8, 2018

I designed a new keyboard, the Comet46, which is a split wireless column staggered keyboard.
I would appreciate it if you could review my code, and add Comet46 to the list of qmk keyboards.

// This is the canonical layout file for the Quantum project. If you want to add another keyboard,

#include QMK_KEYBOARD_H
#include "action_layer.h"
Copy link
Member

Choose a reason for hiding this comment

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

Only the first include should be needed. The other two are redundant.


const uint16_t PROGMEM keymaps[][MATRIX_ROWS][MATRIX_COLS] = {

[_QWERTY] = LAYOUT_kc(
Copy link
Member

Choose a reason for hiding this comment

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

Could you switch the default layout format, rather than the LAYOUT_kc?

The LAYOUT_kc macro seems to cause problems for newer users. It's fine for personal keymaps, or even a default_kc keymap. But for the normal default, we would like to avoid the _kc layouts.

};


void persistent_default_layer_set(uint16_t default_layer) {
Copy link
Member

@drashna drashna Jul 8, 2018

Choose a reason for hiding this comment

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

This isn't needed anymore.

See: #3342 (comment)

switch (keycode) {
case QWERTY:
if (record->event.pressed) {
persistent_default_layer_set(1UL<<_QWERTY);
Copy link
Member

Choose a reason for hiding this comment

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

You can use set_single_persistent_default_layer(_QWERTY); here instead, as it handles this, and some additional stuff.

}
return false;
break;
case LOWER:
Copy link
Member

Choose a reason for hiding this comment

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

The tri layer code has changed, so you may not need to use macros here.

If you use the following, you can use ANY layer change keycode (including LT(x, kc)) to activate the tri layer. The downside is that you can then only get to the 3rd layer (Adjust in this case) via the tri layer. But this may free up keys.

uint32_t layer_state_set_user(uint32_t state) {
  return update_tri_layer_state(state, _RAISE, _LOWER, _ADJUST);
}

This is completely optional though.

@drashna
Copy link
Member

drashna commented Jul 8, 2018

Just to make sure I wasn't misunderstood, you can still have an Adjust layer, and use the tri layer thing (eg, raise + lower = adjust). You just need the layer_state_set_user function to be present. (the default planck keymap has an example of what I mean, as well)

@satt99
Copy link
Contributor Author

satt99 commented Jul 8, 2018

Thanks for taking time reviewing my code.
For the default keymap, I thought having a ADJUST layer won't be necessary because I ended up having only two keys on that layer. (Which are both not mandatory) So, I ended up deleting it.
As for the satt keymap, I'll be using the layer_state_set_user. (still working on it)

@drashna
Copy link
Member

drashna commented Jul 9, 2018

Okay, I had a feeling that was the case, but I wanted to make sure.

Is this PR ready to do?
I'll merge it, if it is. Otherwise, just let me know when it's ready.

@satt99
Copy link
Contributor Author

satt99 commented Jul 9, 2018

Hi drashna, sorry to keep you waiting.
I've modified the satt keymap to use layer_state_set_user and update_tri_layer_state.

The PR should be ready to do now. Thank you so much for your kind support!

@drashna
Copy link
Member

drashna commented Jul 9, 2018

It's not a problem. I just wanted to make sure that I didn't merge it before you were ready!

And thanks!

@drashna drashna merged commit 56b5e9f into qmk:master Jul 9, 2018
@satt99 satt99 deleted the comet46 branch July 9, 2018 15:03
alexey-danilov pushed a commit to alexey-danilov/qmk_firmware that referenced this pull request Jul 27, 2018
* Initial commit for Comet46 firmware

* Update Comet46 README

* Add readme to satt keymap of comet46

* Add default keymap for Comet46

* Fix broken link in readme

* Delete redundant includes

* Modify default keymap & fix LAYOUT macro

* Modify satt keymap of Comet46
ChrissiQ pushed a commit to ChrissiQ/qmk_firmware that referenced this pull request Sep 25, 2018
* Initial commit for Comet46 firmware

* Update Comet46 README

* Add readme to satt keymap of comet46

* Add default keymap for Comet46

* Fix broken link in readme

* Delete redundant includes

* Modify default keymap & fix LAYOUT macro

* Modify satt keymap of Comet46
yamad pushed a commit to yamad/qmk_firmware that referenced this pull request Apr 10, 2019
* Initial commit for Comet46 firmware

* Update Comet46 README

* Add readme to satt keymap of comet46

* Add default keymap for Comet46

* Fix broken link in readme

* Delete redundant includes

* Modify default keymap & fix LAYOUT macro

* Modify satt keymap of Comet46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants