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 crkbd keymap by manna harbour #9461

Closed
wants to merge 1 commit into from

Conversation

manna-harbour
Copy link
Contributor

@manna-harbour manna-harbour commented Jun 18, 2020

This keymap includes crkbd-specific hardware feature support. It provides easy selection of the following:

  • hotswap trackpoint module in OLED port, or OLED with new or old drivers
  • automatic mouse buttons layer activation on trackpoint movement
  • keyboard-side mouse acceleration, or low rates for use with host-side acceleration
  • static image display, or use as caps lock indicator
  • built-in logo, or image from data file, with automatic rotation
  • automatic conversion of image files
  • matrix, light, or underglow RGB

It imports miryoku, an ergonomic, minimal, orthogonal layout for ergo or ortho keyboards.

Description

This PR depends on the following open PRs:

This PR would benefit from the following open PRs:

This PR provides solutions or work-arounds for the following closed issues:

This PR provides enhancements to the following closed PRs:

Types of Changes

  • Keymap/layout/userspace (addition or update)

Checklist

  • My code follows the code style of this project.
  • I have read the CONTRIBUTING document.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@drashna drashna added the awaiting_pr Relies on another PR to be merged first label Jun 18, 2020
@manna-harbour manna-harbour force-pushed the crkbd-pr branch 4 times, most recently from d627a41 to 651dfba Compare June 24, 2020 04:23
@manna-harbour
Copy link
Contributor Author

All of the prerequisite PRs have now been merged and this is now ready for review. Thanks!

@manna-harbour manna-harbour mentioned this pull request Jul 3, 2020
13 tasks
@drashna drashna requested a review from a team July 5, 2020 01:37
@drashna drashna added keymap and removed awaiting_pr Relies on another PR to be merged first labels Jul 5, 2020
@manna-harbour
Copy link
Contributor Author

Thanks @drashna!

This keymap includes crkbd-specific hardware feature support.  It provides easy
selection of the following:

- hotswap trackpoint module in OLED port, or OLED with new or old drivers
- automatic mouse buttons layer activation on trackpoint movement
- keyboard-side mouse acceleration, or low rates for use with host-side
  acceleration
- static image display, or use as caps lock indicator
- built-in logo, or image from data file, with automatic rotation
- automatic conversion of image files
- matrix, light, or underglow RGB

It imports miryoku, an ergonomic, minimal, orthogonal layout for ergo or ortho
keyboards.
Comment on lines +21 to +23
#if defined SSD1306OLED
#include "ssd1306.h"
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, the SSD1306OLED code shouldn't be used, at all. Instead, the oled driver feature should be used in its placen

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#if defined SSD1306OLED
#include "ssd1306.h"
#endif

@tzarc
Copy link
Member

tzarc commented Nov 5, 2021

Can we also get GPL2+ compatible copyright headers on all source files, please?

@@ -0,0 +1,98 @@
// https://github.com/manna-harbour/qmk_firmware/blob/crkbd/keyboards/crkbd/keymaps/manna-harbour/readme.org
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 add a GPL2+ compatible license header here?

For instance:

// Copyright 2021 Your Name (@yourgithub)
// SPDX-License-Identifier: GPL-2.0-or-later

Or

/* Copyright %YEAR% %YOUR_NAME%
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

#if defined RGB_MATRIX_ENABLE
#define RGB_MATRIX_KEYPRESSES
#define RGB_MATRIX_FRAMEBUFFER_EFFECTS
#define RGB_DISABLE_WHEN_USB_SUSPENDED true
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#define RGB_DISABLE_WHEN_USB_SUSPENDED true
#define RGB_DISABLE_WHEN_USB_SUSPENDED

#define RGBLIGHT_HUE_STEP 8
#define RGBLIGHT_SAT_STEP 8
#define RGBLIGHT_VAL_STEP 8
#define RGBLIGHT_ANIMATIONS
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 explicitly define which animations are enabled?

Suggested change
#define RGBLIGHT_ANIMATIONS
#define RGBLIGHT_EFFECT_BREATHING
#define RGBLIGHT_EFFECT_RAINBOW_MOOD
#define RGBLIGHT_EFFECT_RAINBOW_SWIRL
#define RGBLIGHT_EFFECT_SNAKE
#define RGBLIGHT_EFFECT_KNIGHT
#define RGBLIGHT_EFFECT_CHRISTMAS
#define RGBLIGHT_EFFECT_STATIC_GRADIENT
#define RGBLIGHT_EFFECT_RGB_TEST
#define RGBLIGHT_EFFECT_ALTERNATING
#define RGBLIGHT_EFFECT_TWINKLE

@@ -0,0 +1,181 @@
// https://github.com/manna-harbour/qmk_firmware/blob/crkbd/keyboards/crkbd/keymaps/manna-harbour/readme.org
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 add a GPL2+ compatible license header here?

For instance:

// Copyright 2021 Your Name (@yourgithub)
// SPDX-License-Identifier: GPL-2.0-or-later

Or

/* Copyright %YEAR% %YOUR_NAME%
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

Comment on lines +21 to +23
#if defined SSD1306OLED
#include "ssd1306.h"
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#if defined SSD1306OLED
#include "ssd1306.h"
#endif

#endif // defined MH_OLED_IMAGE_FILE && defined OLED_DRIVER_ENABLE


#if defined OLED_DRIVER_ENABLE
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#if defined OLED_DRIVER_ENABLE
#if defined OLED_ENABLE

Comment on lines +137 to +173
#if defined SSD1306OLED

const char *read_logo(void);

void matrix_init_user(void) {
iota_gfx_init(!isLeftHand);
}

void matrix_render_user(struct CharacterMatrix *matrix) {
#if defined MH_OLED_MODE_STATIC
matrix_write(matrix, read_logo());
#elif defined MH_OLED_MODE_CAPS
if (host_keyboard_leds() & (1<<USB_LED_CAPS_LOCK)) {
matrix_write(matrix, read_logo());
}
#endif // defined MH_OLED_MODE_CAPS
}

void matrix_update(struct CharacterMatrix *dest, const struct CharacterMatrix *source) {
if (memcmp(dest->display, source->display, sizeof(dest->display))) {
memcpy(dest->display, source->display, sizeof(dest->display));
dest->dirty = true;
}
}

void iota_gfx_task_user(void) {
struct CharacterMatrix matrix;
matrix_clear(&matrix);
matrix_render_user(&matrix);
matrix_update(&display, &matrix);
}

void matrix_scan_user(void) {
iota_gfx_task();
}

#endif // defined SSD1306OLED
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#if defined SSD1306OLED
const char *read_logo(void);
void matrix_init_user(void) {
iota_gfx_init(!isLeftHand);
}
void matrix_render_user(struct CharacterMatrix *matrix) {
#if defined MH_OLED_MODE_STATIC
matrix_write(matrix, read_logo());
#elif defined MH_OLED_MODE_CAPS
if (host_keyboard_leds() & (1<<USB_LED_CAPS_LOCK)) {
matrix_write(matrix, read_logo());
}
#endif // defined MH_OLED_MODE_CAPS
}
void matrix_update(struct CharacterMatrix *dest, const struct CharacterMatrix *source) {
if (memcmp(dest->display, source->display, sizeof(dest->display))) {
memcpy(dest->display, source->display, sizeof(dest->display));
dest->dirty = true;
}
}
void iota_gfx_task_user(void) {
struct CharacterMatrix matrix;
matrix_clear(&matrix);
matrix_render_user(&matrix);
matrix_update(&display, &matrix);
}
void matrix_scan_user(void) {
iota_gfx_task();
}
#endif // defined SSD1306OLED


OPT_DEFS += -DMH_USER_NAME_H=\"$(USER_NAME).h\"

ifeq ($(strip $(MH_MODULE)), trackpoint)
Copy link
Member

Choose a reason for hiding this comment

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

All of this should probably go in post_rules.mk

@@ -0,0 +1,40 @@
// https://github.com/manna-harbour/qmk_firmware/blob/crkbd/keyboards/crkbd/keymaps/manna-harbour/readme.org
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 add a GPL2+ compatible license header here?

For instance:

// Copyright 2021 Your Name (@yourgithub)
// SPDX-License-Identifier: GPL-2.0-or-later

Or

/* Copyright %YEAR% %YOUR_NAME%
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

@@ -0,0 +1,40 @@
// https://github.com/manna-harbour/qmk_firmware/blob/crkbd/keyboards/crkbd/keymaps/manna-harbour/readme.org
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 add a GPL2+ compatible license header here?

For instance:

// Copyright 2021 Your Name (@yourgithub)
// SPDX-License-Identifier: GPL-2.0-or-later

Or

/* Copyright %YEAR% %YOUR_NAME%
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

@keyboard-magpie
Copy link
Contributor

@manna-harbour in the spirit of tidying up, does this PR still need to remain open, or does your current mechanism fulfil all needs? IMO the latter is true.

@manna-harbour
Copy link
Contributor Author

Thanks @drashna and @tzarc for the review.

This PR is now very out of date, and I've found a different method of adding custom code to any keymap at manna-harbour#40, which should be much better than the USER_NAME method used here. The features from this PR will be added there in future, and I'll be sure to incorporate your feedback from this PR at that time.

As an aside, there was 12 months between @drashna's original approval and request for review, and the next review. There may be an issue with the use of the awaiting review label, as it suppresses the stale action, which seems to be the only automated reminder for PRs needing attention. Perhaps an awaiting review reminder action is also needed?

Thanks @keyboard-magpie for the reminder. The Miryoku customisation method is separate from this, but even so, this PR is now obsolete. Closing now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants