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

feat(shields): Add MechWild BB65 #1714

Open
wants to merge 8 commits into
base: main
Choose a base branch
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)

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for these settings to be in the board-specific file (and always enabled unless disabled explicitly) rather than in bb65.conf? Also applies to #1713.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The logic here is that if you use an alternate board for this shield (such as the blackpill, which is supported) the pin defs wont be the same, so it will not have the pillbug.overlay file to reference and will throw errors without the feature being broken out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, that makes sense. My only concern would be that it is undiscoverable for users if they want to disable it, since it doesn't appear in the files that get copied to the user config repo. If that is something you'd want, you could add a commented out CONFIG_ZMK_RGB_UNDERGLOW=n so at least it is more discoverable. (If you do that, you might also want to test the build to make sure it will actually override the setting if uncommented.)

(I'd also tend towards not enabling optional features by default but I guess that's a vendor/maintainer decision.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see the point you are making. I based this off a fairly old way of approaching this. I will make the changes to put the conf into the main file and not keep it restricted here. This will proc issues in the case where someone wants to build with a blackpill and tries to enable it, but I suppose I can just do the thing I wanted to avoid and define those boards as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wanted to avoid because lazy and I don't have one built right now to use it so I can't test the light defs. I can do it later though and add to the PR.

Copy link
Contributor

@caksoylar caksoylar Mar 17, 2023

Choose a reason for hiding this comment

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

I think it'd be good for more blackpill support in ZMK, but I don't necessarily want to force you one way or the other! If you just added a disabling config in bb65.conf that wouldn't necessitate adding support for other blackpills, unless the users changed the line to enable it =y, rather than just uncommenting.

Worth noting that we have the longstanding issue #885 related to board vs. shield-specific settings for underglow and it affects more common board/shield combinations (e.g. nice_nano_v2 and corne).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh the change to mapped pins would be perfect here too. I wanted to add the blackpill support for these boards when I did this PR, but got lazy since I can't find my screwdriver. Hell, they are the reason I added the blackpill defs to the repo in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

Accidental, but you'll want to revert this change.

Comment on lines +9 to +25
if LVGL

config LV_Z_VDB_SIZE
default 64

config LV_DPI_DEF
default 148

config LV_Z_BITS_PER_PIXEL
default 1

choice LV_COLOR_DEPTH
default LV_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 LV_Z_VDB_SIZE
default 64
config LV_DPI_DEF
default 148
config LV_Z_BITS_PER_PIXEL
default 1
choice LV_COLOR_DEPTH
default LV_COLOR_DEPTH_1
endchoice
endif # LVGL

Are these necessary on a board with no display?

Comment on lines +9 to +15
&spi1 {
compatible = "nordic,nrf-spim";
status = "okay";
mosi-pin = <5>;
// Unused pins, needed for SPI definition, but not used by the ws2812 driver itself.
sck-pin = <27>;
miso-pin = <28>;
Copy link
Contributor

@lesshonor lesshonor May 11, 2023

Choose a reason for hiding this comment

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

This will need to be updated to use &pinctrl and &spi3 nodes like the other shields in the repo for RGB to work on pillbug. (As there is no worldsemi,ws2812-pwm implementation in Zephyr for blackpill_f4X1Xe, RGB seems to be a no-go on those boards at this point in time.)

Please verify that it works on your hardware.

Suggested change
&spi1 {
compatible = "nordic,nrf-spim";
status = "okay";
mosi-pin = <5>;
// 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, 5)>;
};
};
spi3_sleep: spi3_sleep {
group1 {
psels = <NRF_PSEL(SPIM_MOSI, 0, 5)>;
low-power-enable;
};
};
};
&spi3 {
compatible = "nordic,nrf-spim";
status = "okay";
pinctrl-0 = <&spi3_default>;
pinctrl-1 = <&spi3_sleep>;
pinctrl-names = "default", "sleep";

@caksoylar caksoylar added the shields PRs and issues related to shields label Jun 10, 2023
@caksoylar caksoylar mentioned this pull request Jun 10, 2023
11 tasks
bindings = <&bt BT_NXT>;
};
};

Copy link
Contributor

@lesshonor lesshonor Oct 17, 2023

Choose a reason for hiding this comment

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

Suggested change

Spacing. Installing and running pre-commit can help catch this stuff.

&kp LCTRL &kp LGUI &kp LALT &kp SPACE &kp RALT &mo FN &kp RCTRL &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
};
};

&trans &trans &trans &trans &trans &trans &trans &trans &trans &trans &trans &trans &trans &trans
&trans &trans &trans &trans &trans &trans &trans &trans &trans &trans &trans &trans &trans &kp PG_UP &ext_power EP_TOG
&trans &trans &trans &trans &trans &trans &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
>;
>;

&trans &trans &trans &trans &trans &trans &trans &trans &trans &trans &trans &trans &trans &trans
&trans &trans &trans &trans &trans &trans &bootloader &trans &trans &trans &trans &trans &trans &trans &rgb_ug RGB_EFF
&trans &out OUT_BLE &out OUT_USB &trans &trans &trans &trans &trans &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.

Suggested change
>;
>;

&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
>;
>;

label = "Encoder 1";
a-gpios = <&blackpill 12 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
b-gpios = <&blackpill 13 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
resolution = <4>;
Copy link
Contributor

@lesshonor lesshonor Oct 17, 2023

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>;

This change is part of the big encoder refactor circa June 2023.

Comment on lines +55 to +58
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>;
};

See comment.

a-gpios = <&blackpill 12 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
b-gpios = <&blackpill 13 (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.

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

The new attribute is also part of the encoder refactor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
shields PRs and issues related to shields
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants