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

Update and add bioi keyboards #20485

Closed
wants to merge 8 commits into from

Conversation

scottywei
Copy link

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: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • 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).

@github-actions github-actions bot added keyboard keymap via Adds via keymap and/or updates keyboard for via support labels Apr 18, 2023
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.

There are quite a lot of issues, a lot of which are covered by the PR checklist.

Given this, you should probably submit keyboards as individual PRs, one after another.

@zvecr zvecr added the pr_checklist_pending Needs changes as per the PR checklist label Apr 18, 2023
@scottywei scottywei force-pushed the update-and-add-bioi-keyboards branch from 17b268a to 20533ee Compare April 18, 2023 18:29
@scottywei scottywei force-pushed the update-and-add-bioi-keyboards branch from 20533ee to 8fe00f0 Compare April 18, 2023 19:02
@scottywei
Copy link
Author

There are quite a lot of issues, a lot of which are covered by the PR checklist.

Given this, you should probably submit keyboards as individual PRs, one after another.

Hi, Thank you for the reminding, I have go through the checklist and updated some codes according to my knowledge. Most of the keyboard in this PR is relied on the updated main.c and ble.c in this PR too, So I thought they should go together in one PR. Since the code is quite a lot, any problem you found in one keyboard I would check and fix it in the others. Thank you agian for your hard work on this great project!

@scottywei scottywei requested a review from zvecr April 19, 2023 15:54
@8va
Copy link

8va commented May 12, 2023

@zvecr just a gentle reminder on this. would love to see the included keyboards in this merged to QMK. thanks dude!

@drashna
Copy link
Member

drashna commented May 14, 2023

166 files ... is a lot.

Also, #20897 changes this code as well.

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.

And same can/should be applied to all of the rest of the boards. But ideally, it should be split into separate PRs, so that it's easier to review, rather than a single, massive PR.


#define KEYBOARD_LOCK_ENABLE
#define MAGIC_KEY_LOCK L
#define IS_COMMAND() (get_mods() == (MOD_BIT(KC_LSFT) | MOD_BIT(KC_RSFT)))
Copy link
Member

Choose a reason for hiding this comment

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

This is the default option.

Suggested change
#define IS_COMMAND() (get_mods() == (MOD_BIT(KC_LSFT) | MOD_BIT(KC_RSFT)))

Comment on lines +21 to +24
#define MATRIX_ROW_PINS { D4, B3 }
#define MATRIX_COL_PINS { D0, D1, D5, B0, B2, D6 }

#define DIODE_DIRECTION COL2ROW
Copy link
Member

Choose a reason for hiding this comment

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

Should be moved to info.json.

#define RGBLIGHT_LIMIT_VAL 127

#define RGB_MATRIX_KEYRELEASES
#define RGB_DI_PIN B7
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_DI_PIN B7
#define WS2812_DI_PIN B7
#define RGB_DI_PIN WS2812_DI_PIN

Comment on lines +19 to +21
#include "print.h"
#include "../../../ble.h"
#include "quantum.h"
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be required in the keymap.c.

Suggested change
#include "print.h"
#include "../../../ble.h"
#include "quantum.h"

Comment on lines +1 to +9
MCU = atmega32u4
F_CPU = 8000000
ARCH = AVR8
F_USB = $(F_CPU)
BOOTLOADER = qmk-dfu
BOOTLOADER_SIZE = 4096


OPT_DEFS += -DINTERRUPT_CONTROL_ENDPOINT
Copy link
Member

Choose a reason for hiding this comment

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

This can be removed. Just the processor and bootloader need to be specified, and in the info.json file.

Copy link
Author

@scottywei scottywei Jun 18, 2023

Choose a reason for hiding this comment

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

Some of the design is using ATmega32u4 under [email protected], I can't find any document that instructing me how to define that in a info.json, and some design using a custom built qmk-dfu bootloader with a size of 8192, I think I should keep some definition here.

Comment on lines +19 to +34
# Build Options
BOOTMAGIC_ENABLE = no
MOUSEKEY_ENABLE = no
EXTRAKEY_ENABLE = yes
CONSOLE_ENABLE = no
COMMAND_ENABLE = yes
NKRO_ENABLE = no
BACKLIGHT_ENABLE = no
AUDIO_ENABLE = no
UNICODE_ENABLE = no
BLUETOOTH_ENABLE = no
RGB_MATRIX_ENABLE = yes
RGB_MATRIX_DRIVER = WS2812
SLEEP_LED_ENABLE = no
LTO_ENABLE = yes
VIA_ENABLE = yes
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 be moved to the info.json.

Also, via should not be enabled at the kb level, and needs to be enabled only at the keymap level.


bool process_record_user(uint16_t keycode, keyrecord_t *record) {
switch (keycode) {
case KC_F13:
Copy link
Member

Choose a reason for hiding this comment

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

There is no need for custom keycodes here, for the rgb matrix control.

Comment on lines +68 to +87
case KC_F21:
if (record->event.pressed) {
module_reset();
} else {
}
break;
case KC_F19:
if (record->event.pressed) {
reset_ble_batt();

} else {
}
break;
case KC_F20:
if (record->event.pressed) {
update_ble_batt();
wait_ms(100);

} else {
}
Copy link
Member

Choose a reason for hiding this comment

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

These should be custom keycodes, NOT using existing keycodes.

Copy link
Member

Choose a reason for hiding this comment

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

This file can be removed, and should be moved to info.json

Copy link
Author

Choose a reason for hiding this comment

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

How could I get VIA support if che keyboard.h is removed?

Copy link
Author

Choose a reason for hiding this comment

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

And... may I ask why we should remove this file, which have a matrix macro that can be easily read, and check with keyboard matrix schematic, matrix in a JSON is quite unreadable for hardware designer and difficult to check with schematic.

Copy link
Contributor

@lesshonor lesshonor Jun 19, 2023

Choose a reason for hiding this comment

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

How could I get VIA support if che keyboard.h is removed?

The existence of this file is unrelated to the existence of a via keymap folder. You may examine any keyboard in this repository with a via keymap (example) for a better understanding of how this works.

may I ask why we should remove this file

As noted in the PR checklist:

layout definitions must include matrix positions, so that LAYOUT macros can be generated at build time

Much code is now being automatically generated at build time from static data. Validation is also being added so incorrect/broken configurations can be found earlier. Parsing C to accomplish this task is much more involved than parsing JSON.

qmk info -f json -kb keyboardname can help with the conversion, but do check other keyboards such as the one linked above to remove redundant attributes.
For a visual representation of the keyboard, run qmk info -m -kb keyboardname, qmk info -l -kb keyboardname, and/or upload the info.json file to QMK Configurator.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you so much for the instruction, it's really helpful!

@github-actions
Copy link

github-actions bot commented Aug 4, 2023

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Aug 4, 2023
@github-actions
Copy link

github-actions bot commented Sep 4, 2023

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.
// [stale-action-closed]

@github-actions github-actions bot closed this Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keyboard keymap pr_checklist_pending Needs changes as per the PR checklist stale Issues or pull requests that have become inactive without resolution. via Adds via keymap and/or updates keyboard for via support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants