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

feat(shields): Add MechWild OBE #1713

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat(shields): Add MechWild OBE #1713

wants to merge 5 commits into from

Conversation

kylemccreery
Copy link
Contributor

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)

@caksoylar caksoylar mentioned this pull request Mar 17, 2023
Open
11 tasks
caksoylar
caksoylar requested changes Jun 10, 2023
View reviewed changes
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.

FYI, this shield seems to need updating to Zephyr 3.2. @lesshonor also has some relevant comments in #1714 related to pinctrl.

@caksoylar caksoylar added the shields PRs and issues related to shields label Jun 10, 2023
lesshonor
lesshonor suggested changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

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

Much of the suggestions here are identical to ones made in #1714, so if something isn't explained here, it is explained there.

You're going to have to rebase on main in order for this to build. (...or merge, I guess).

Squashing your commits down to one and rewriting the message so it adheres to Conventional Commit guidelines may also make the prospect of merging more attractive to the powers that be, but that's a bit of a guess on my part. (It certainly couldn't hurt.)

app/boards/shields/obe/Kconfig.defconfig
Comment on lines +8 to +24

if LVGL

config LVGL_VDB_SIZE
default 64

config LVGL_DPI
default 148

config LVGL_BITS_PER_PIXEL
default 1

choice LVGL_COLOR_DEPTH
default LVGL_COLOR_DEPTH_1
endchoice

endif # LVGL
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
if LVGL
config LVGL_VDB_SIZE
default 64
config LVGL_DPI
default 148
config LVGL_BITS_PER_PIXEL
default 1
choice LVGL_COLOR_DEPTH
default LVGL_COLOR_DEPTH_1
endchoice
endif # LVGL

No display on this board.

app/boards/shields/obe/obe.keymap
combo_reset {
timeout-ms = <TIMEOUT>;
key-positions = <1 3>;
bindings = <&reset>;
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
bindings = <&reset>;
bindings = <&sys_reset>;

This is part of the Zephyr 3.2 update circa April 2023.

app/boards/shields/obe/obe.keymap
&kp LCTRL &kp LGUI &kp LALT &kp SPACE &mo 1 &kp SPACE &kp RALT &mo 1 &kp LEFT &kp DOWN &kp RIGHT
>;
sensor-bindings = <&inc_dec_kp C_VOL_UP C_VOL_DN>;
};
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
};
};

app/boards/shields/obe/obe.keymap
&bt BT_NXT &kp CAPS &kp KP_NUM &kp SLCK &trans &trans &trans &trans &trans &trans &trans &trans &trans &trans
&trans &trans &bt BT_CLR &trans &trans &trans &trans &trans &trans &trans &trans &trans &kp PG_UP &trans
&trans &trans &rgb_ug RGB_BRD &rgb_ug RGB_EFR &ext_power EP_TOG &rgb_ug RGB_EFF &rgb_ug RGB_BRI &trans &kp HOME &kp PG_DN &kp END
>;
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
>;
>;

app/boards/shields/obe/obe.keymap
&none &none &none &none &none &none &none &none &none &none &none &none &none &none
&none &none &none &none &none &none &none &none &none &none &none &none &none &none
&none &none &none &none &none &none &none &none &none &none &none
>;
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
>;
>;

app/boards/shields/obe/obe.overlay
label = "Encoder 1";
a-gpios = <&blackpill 40 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
b-gpios = <&blackpill 41 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
resolution = <4>;
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
resolution = <4>;
steps = <80>;

app/boards/shields/obe/boards/pillbug.overlay
Comment on lines +9 to +15
&spi1 {
compatible = "nordic,nrf-spim";
status = "okay";
mosi-pin = <25>;
// Unused pins, needed for SPI definition, but not used by the ws2812 driver itself.
sck-pin = <27>;
miso-pin = <28>;
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
&spi1 {
compatible = "nordic,nrf-spim";
status = "okay";
mosi-pin = <25>;
// Unused pins, needed for SPI definition, but not used by the ws2812 driver itself.
sck-pin = <27>;
miso-pin = <28>;
&pinctrl {
spi3_default: spi3_default {
group1 {
psels = <NRF_PSEL(SPIM_MOSI, 0, 25)>;
};
};
spi3_sleep: spi3_sleep {
group1 {
psels = <NRF_PSEL(SPIM_MOSI, 0, 25)>;
low-power-enable;
};
};
};
&spi3 {
compatible = "nordic,nrf-spim";
status = "okay";
pinctrl-0 = <&spi3_default>;
pinctrl-1 = <&spi3_sleep>;
pinctrl-names = "default", "sleep";

lesshonor
lesshonor suggested changes Nov 14, 2023
View reviewed changes
app/boards/shields/obe/obe.overlay
a-gpios = <&blackpill 40 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
b-gpios = <&blackpill 41 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
resolution = <4>;
status = "disabled";
Copy link
Contributor

Choose a reason for hiding this comment

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

I have it on good authority that the sensors node actually belongs in the overlay and should be referred to from the keymap if necessary.

Suggested change
status = "disabled";
status = "disabled";
sensors {
compatible = "zmk,keymap-sensors";
sensors = <&encoder_1>;
triggers-per-rotation = <20>;
};

app/boards/shields/obe/obe.keymap
Comment on lines +48 to +52

sensors {
compatible = "zmk,keymap-sensors";
sensors = <&encoder_1>;
};
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
sensors {
compatible = "zmk,keymap-sensors";
sensors = <&encoder_1>;
};

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

@caksoylar caksoylar caksoylar requested changes

@lesshonor lesshonor lesshonor requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
shields PRs and issues related to shields
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kylemccreery @caksoylar @lesshonor