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

Added Waterfowl shield #1554

Merged
merged 22 commits into from Dec 11, 2022
Merged

Added Waterfowl shield #1554

merged 22 commits into from Dec 11, 2022

Conversation

JW2586
Copy link
Contributor

@JW2586 JW2586 commented Nov 25, 2022

Board/Shield Check-list

  • This board/shield is tested working on real hardware
  • Definitions follow the general style of other shields/boards upstream (Reference)
  • .zmk.yml metadata file added
  • Proper Copyright + License headers added to applicable files (Generally, we stick to "The ZMK Contributors" for copyrights to help avoid churn when files get edited)
  • General consistent formatting of DeviceTree files
  • Keymaps do not use deprecated key defines (Check using the upgrader tool)
  • &pro_micro used in favor of &pro_micro_d/a if applicable
  • If split, no name added for the right/peripheral half
  • Kconfig.defconfig file correctly wraps all configuration in conditional on the shield symbol
  • .conf file has optional extra features commented out
  • Keyboard/PCB is part of a shipped group buy or is generally available in stock to purchase (OSH/personal projects without general availability should create a zmk-config repo instead)

Copy link
Contributor

@caksoylar caksoylar left a comment

Choose a reason for hiding this comment

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

Have you tested this with OLEDs on a recent ZMK version? It is missing some config that I noted in the comments, I believe without those it wouldn't work.

config ZMK_KEYBOARD_NAME
default "Waterfowl"

config ZMK_SPLIT_BLE_ROLE_CENTRAL
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
config ZMK_SPLIT_BLE_ROLE_CENTRAL
config ZMK_SPLIT_ROLE_CENTRAL


config LVGL_VER_RES_MAX
default 64

Copy link
Contributor

Choose a reason for hiding this comment

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

The two resolution Kconfigs are no longer necessary since da209c4

RC(0,0) RC(0,1) RC(0,2) RC(0,3) RC(0,4) RC(0,5) RC(0,6) RC(0,7) RC(0,8) RC(0,9)
RC(1,0) RC(1,1) RC(1,2) RC(1,3) RC(1,4) RC(1,5) RC(1,6) RC(1,7) RC(1,8) RC(1,9)
RC(2,0) RC(2,1) RC(2,2) RC(2,3) RC(2,4) RC(2,5) RC(2,6) RC(2,7) RC(2,8) RC(2,9)
RC(3,0) RC(3,1) RC(3,2) RC(3,3) RC(3,4) RC(3,5) RC(3,6) RC(3,7) RC(3,8) RC(3,9)
Copy link
Contributor

Choose a reason for hiding this comment

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

There appears to be space/tab mixup on the above 9 lines, I'd stick to one or the other.

&pro_micro_i2c {
status = "okay";

ssd1306@3c {
Copy link
Contributor

@caksoylar caksoylar Nov 26, 2022

Choose a reason for hiding this comment

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

You need to add a label to this node (oled: ssd1306@3c) then use it in the chosen node above like zephyr,display = &oled;, see example at https://github.com/zmkfirmware/zmk/blob/main/app/boards/shields/corne/corne.dtsi#L11

&kp Q &kp W &kp E &kp R &kp T &kp Y &kp U &kp I &kp O &kp P
&mt LGUI A &mt LALT S &mt LCTRL D &mt LSHFT F &kp G &kp H &mt LSHFT J &mt LCTRL K &mt LALT L &mt LGUI SEMI
&kp Z &kp X &kp C &kp V &kp B &kp N &kp M &kp COMMA &kp DOT &kp FSLH
&kp N1 &lt 3 DEL &lt 1 SPACE &kp TAB &kp N2 &kp N3 &kp ESC &kp BSPC &lt 2 RET &kp N4
Copy link
Contributor

Choose a reason for hiding this comment

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

Some tab/space mixup between left/right blocks in the layer definitions as well, it throws off the alignment when tabstop is set to 8 (Github default)

* `-----' `--------------------' `--------------------' `-----'
*/
bindings = <
&kp PRCNT &kp LS(SQT) &kp LBKT &kp RBKT &kp NON_US_BSLH &kp RA(GRAVE) &kp LS(GRAVE) &kp CARET &trans &trans
Copy link
Contributor

Choose a reason for hiding this comment

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

The default keymap seems to be set for a UK layout and this is unusual compared to the other board/shields in the repo. I wanted to point it out but I don't really have an opinion one way or the other on whether this is wrong or not. It will not work as expected with US keyboard layouts.

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

Thanks!

@petejohanson petejohanson added enhancement New feature or request shields PRs and issues related to shields split labels Dec 10, 2022
@petejohanson petejohanson merged commit 617136b into zmkfirmware:main Dec 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request shields PRs and issues related to shields split
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants